[1/6] drm: bridge: samsung-dsim: Support multi-lane calculations

If there is more than one lane, the HFP, HBP, and HSA is calculated in
bytes/pixel, then they are divided amongst the different lanes with some
additional overhead. This is necessary to achieve higher resolutions while
keeping the pixel clocks lower as the number of lanes increase.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 40 +++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 6 deletions(-)

On 4/15/23 12:40, Adam Ford wrote:
> If there is more than one lane, the HFP, HBP, and HSA is calculated in
> bytes/pixel, then they are divided amongst the different lanes with some
> additional overhead. This is necessary to achieve higher resolutions while
> keeping the pixel clocks lower as the number of lanes increase.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 40 +++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..1ccbad4ea577 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -215,6 +215,7 @@
>   #define DSI_RX_FIFO_SIZE		256
>   #define DSI_XFER_TIMEOUT_MS		100
>   #define DSI_RX_FIFO_EMPTY		0x30800002
> +#define DSI_HSYNC_PKT_OVERHEAD	6
>   
>   #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
>   
> @@ -879,13 +880,40 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>   			| DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
>   		samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
>   
> -		reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> -			| DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> -		samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> +		/*
> +		 * If there is more than one lane, the HFP, HBP, and HSA
> +		 * is calculated in bytes/pixel, then they are divided
> +		 * amongst the different lanes with some additional
> +		 * overhead correction
> +		 */

Did you find any confirmation of this in the MX8M* datasheet or at least 
by measuring the DSI data lanes with a scope ?

It would be real cool if this could be confirmed somehow, and we could 
rule out that this tweaking of HSA/HSE/... stuff isn't related to either 
LP-HS transition timing calculation this driver is missing, OR, 
incorrect flags in various bridge/panel drivers like commit:

ca161b259cc84 ("drm/bridge: ti-sn65dsi83: Do not generate HFP/HBP/HSA 
and EOT packet")

On 4/15/23 12:40, Adam Ford wrote:
> According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> and max values for M and  the frequency range for the VCO_out
> calculator were incorrect.  This also appears to be the case for the
> imx8mn and imx8mp.
> 
> To fix this, make new variables to hold the min and max values of m
> and the minimum value of VCO_out, and update the PMS calculator to
> use these new variables instead of using hard-coded values to keep
> the backwards compatibility with other parts using this driver.

[...]

>   static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> @@ -470,6 +485,9 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
>   	 */
>   	.pll_p_offset = 14,
>   	.reg_values = imx8mm_dsim_reg_values,
> +	.m_min = 64,
> +	.m_max = 1023,
> +	.vco_min = 1050,

You might want to call this 'min_freq' since there is a 'max_freq' which 
seems to indicate what VCO max frequency is.

Note that the same datasheet contains the following information:
"
MIPI_DPHY_M_PLLPMS field descriptions

12–4 PMS_M
Specifies the PLL PMS value for the M divider
NOTE: The programmable divider range should be within 25 to 125 to 
ensure PLL stability.
NOTE: The M and P divider values should be considered together to ensure 
VCO ouput frequency
(VCO_out) range is between 350 MHz to 750 MHz.
Please refer to the topic DPHY PLL for more information.
"

On 4/15/23 12:41, Adam Ford wrote:
> NXP uses a lookup table to determine the various values for
> the PHY Timing based on the clock rate in their downstream
> kernel.  Since the input clock can be variable, the phy
> settings need to be variable too.  Add an additional variable
> to the driver data to enable this feature to prevent breaking
> boards that don't support it.

Isn't there already a generic LP2HS transition time calculation in the 
kernel ?

This looks like a generic calculation which should be done by all the 
DSI hosts, so why not introduce a generic helper to do such a 
calculation (and not a table please) ?

On Sun, Apr 16, 2023 at 5:07 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/15/23 12:40, Adam Ford wrote:
> > According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> > and max values for M and  the frequency range for the VCO_out
> > calculator were incorrect.  This also appears to be the case for the
> > imx8mn and imx8mp.
> >
> > To fix this, make new variables to hold the min and max values of m
> > and the minimum value of VCO_out, and update the PMS calculator to
> > use these new variables instead of using hard-coded values to keep
> > the backwards compatibility with other parts using this driver.
>
> [...]
>
> >   static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> > @@ -470,6 +485,9 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> >        */
> >       .pll_p_offset = 14,
> >       .reg_values = imx8mm_dsim_reg_values,
> > +     .m_min = 64,
> > +     .m_max = 1023,
> > +     .vco_min = 1050,
>
> You might want to call this 'min_freq' since there is a 'max_freq' which
> seems to indicate what VCO max frequency is.
>
> Note that the same datasheet contains the following information:
> "
> MIPI_DPHY_M_PLLPMS field descriptions
>
> 12–4 PMS_M
> Specifies the PLL PMS value for the M divider
> NOTE: The programmable divider range should be within 25 to 125 to
> ensure PLL stability.

I was confused by this because this statement is not consistent with
the link they reference jumps me to the table where it reads M is
between 64 and 1023.

> NOTE: The M and P divider values should be considered together to ensure
> VCO ouput frequency
> (VCO_out) range is between 350 MHz to 750 MHz.
> Please refer to the topic DPHY PLL for more information.

I was confused by this too, because the NXP documentation reads the
350 - 750MHz that you state, but  "Table 13-45: DPHY PLL Parameters"
which immediately follows that sentence  on page 4158 shows VCO_out is
between 1050-2100 MHz.

I compared the PMS values for a variety of frequencies to those that
were set in the downstream NXP code, and the PMS values matched.
Maybe someone from NXP can explain the discrepancy.

adam

> "

Hi,

Am Montag, 17. April 2023, 00:31:24 CEST schrieb Adam Ford:
> On Sun, Apr 16, 2023 at 5:07 PM Marek Vasut <marex@denx.de> wrote:
> > On 4/15/23 12:40, Adam Ford wrote:
> > > According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> > > and max values for M and  the frequency range for the VCO_out
> > > calculator were incorrect.  This also appears to be the case for the
> > > imx8mn and imx8mp.
> > > 
> > > To fix this, make new variables to hold the min and max values of m
> > > and the minimum value of VCO_out, and update the PMS calculator to
> > > use these new variables instead of using hard-coded values to keep
> > > the backwards compatibility with other parts using this driver.
> > 
> > [...]
> > 
> > >   static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data =
> > >   {
> > > 
> > > @@ -470,6 +485,9 @@ static const struct samsung_dsim_driver_data
> > > imx8mm_dsi_driver_data = {> > 
> > >        */
> > >       
> > >       .pll_p_offset = 14,
> > >       .reg_values = imx8mm_dsim_reg_values,
> > > 
> > > +     .m_min = 64,
> > > +     .m_max = 1023,
> > > +     .vco_min = 1050,
> > 
> > You might want to call this 'min_freq' since there is a 'max_freq' which
> > seems to indicate what VCO max frequency is.
> > 
> > Note that the same datasheet contains the following information:
> > "
> > MIPI_DPHY_M_PLLPMS field descriptions
> > 
> > 12–4 PMS_M
> > Specifies the PLL PMS value for the M divider
> > NOTE: The programmable divider range should be within 25 to 125 to
> > ensure PLL stability.
> 
> I was confused by this because this statement is not consistent with
> the link they reference jumps me to the table where it reads M is
> between 64 and 1023.
> 
> > NOTE: The M and P divider values should be considered together to ensure
> > VCO ouput frequency
> > (VCO_out) range is between 350 MHz to 750 MHz.
> > Please refer to the topic DPHY PLL for more information.
> 
> I was confused by this too, because the NXP documentation reads the
> 350 - 750MHz that you state, but  "Table 13-45: DPHY PLL Parameters"
> which immediately follows that sentence  on page 4158 shows VCO_out is
> between 1050-2100 MHz.
> 
> I compared the PMS values for a variety of frequencies to those that
> were set in the downstream NXP code, and the PMS values matched.
> Maybe someone from NXP can explain the discrepancy.

Also note that, according to Table 13-28 in RM (Rev 2 07/2022) for i.MX8M 
Nano, VCO_out and Fout has a maximum of 1500MHz only. Although the note above 
mentions a range  of 1050-2100MHz...

Best regards,
Alexander

Hi Adam,

Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> If there is more than one lane, the HFP, HBP, and HSA is calculated in
> bytes/pixel, then they are divided amongst the different lanes with some
> additional overhead. This is necessary to achieve higher resolutions while
> keeping the pixel clocks lower as the number of lanes increase.
> 

In the testing I did to come up with my patch "drm: bridge: samsung-
dsim: fix blanking packet size calculation" the number of lanes didn't
make any difference. My testing might be flawed, as I could only
measure the blanking after translation from MIPI DSI to DPI, so I'm
interested to know what others did here. How did you validate the
blanking with your patch? Would you have a chance to test my patch and
see if it works or breaks in your setup?

Regards,
Lucas

> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 40 +++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..1ccbad4ea577 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -215,6 +215,7 @@
>  #define DSI_RX_FIFO_SIZE		256
>  #define DSI_XFER_TIMEOUT_MS		100
>  #define DSI_RX_FIFO_EMPTY		0x30800002
> +#define DSI_HSYNC_PKT_OVERHEAD	6
>  
>  #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
>  
> @@ -879,13 +880,40 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
>  			| DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
>  		samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
>  
> -		reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> -			| DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> -		samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> +		/*
> +		 * If there is more than one lane, the HFP, HBP, and HSA
> +		 * is calculated in bytes/pixel, then they are divided
> +		 * amongst the different lanes with some additional
> +		 * overhead correction
> +		 */
> +		if (dsi->lanes > 1) {
> +			u32 hfp, hbp, hsa;
> +			int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format) / 8;
> +
> +			hfp = ((m->hsync_start - m->hdisplay) * bpp) / dsi->lanes;
> +			hfp -= (hfp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> +
> +			hbp = ((m->htotal - m->hsync_end) * bpp) / dsi->lanes;
> +			hbp -= (hbp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
>  
> -		reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> -			| DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> -		samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> +			hsa = ((m->hsync_end - m->hsync_start) * bpp) / dsi->lanes;
> +			hsa -= (hsa > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> +
> +			reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
> +			samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> +
> +			reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> +				| DSIM_MAIN_HSA(hsa);
> +			samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> +		} else {
> +			reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> +				| DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> +			samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> +
> +			reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> +				| DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> +			samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> +		}
>  	}
>  	reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
>  		DSIM_MAIN_VRESOL(m->vdisplay, num_bits_resol);

On Mon, Apr 17, 2023 at 3:38 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Adam,
>
> Am Samstag, dem 15.04.2023 um 05:41 -0500 schrieb Adam Ford:
> > NXP uses a lookup table to determine the various values for
> > the PHY Timing based on the clock rate in their downstream
> > kernel.  Since the input clock can be variable, the phy
> > settings need to be variable too.  Add an additional variable
> > to the driver data to enable this feature to prevent breaking
> > boards that don't support it.
> >
>
> I haven't checked if this generates values close to the ones in this
> table, but I guess it should be worth a try to use
> phy_mipi_dphy_get_default_config() instead.

I didn't know that was a thing.  I like that idea much better than the
table.  I just pulled what NXP had and tweaked it to fit the mainline.
I'll give it a try in the next few days, when I have some more time.

adam
>
> Regards,
> Lucas
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c |  85 +++++++--
> >  drivers/gpu/drm/bridge/samsung-dsim.h | 254 ++++++++++++++++++++++++++
> >  include/drm/bridge/samsung-dsim.h     |   1 +
> >  3 files changed, 326 insertions(+), 14 deletions(-)
> >  create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.h
> >
<snip>
>

On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Adam,
>
> Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> > If there is more than one lane, the HFP, HBP, and HSA is calculated in
> > bytes/pixel, then they are divided amongst the different lanes with some
> > additional overhead. This is necessary to achieve higher resolutions while
> > keeping the pixel clocks lower as the number of lanes increase.
> >
>
> In the testing I did to come up with my patch "drm: bridge: samsung-
> dsim: fix blanking packet size calculation" the number of lanes didn't
> make any difference. My testing might be flawed, as I could only
> measure the blanking after translation from MIPI DSI to DPI, so I'm
> interested to know what others did here. How did you validate the
> blanking with your patch? Would you have a chance to test my patch and
> see if it works or breaks in your setup?

Mine was purely by trial and error.  I don't have a scope, nor do I
have a copy of the MIPI DSI spec, so if the image sync'd with my
monitor, I treated it as successful.

I can give yours a try, but it might be a few days since I've only
been working on this stuff a bit in my spare time.

adam

>
> Regards,
> Lucas
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 40 +++++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index e0a402a85787..1ccbad4ea577 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -215,6 +215,7 @@
> >  #define DSI_RX_FIFO_SIZE             256
> >  #define DSI_XFER_TIMEOUT_MS          100
> >  #define DSI_RX_FIFO_EMPTY            0x30800002
> > +#define DSI_HSYNC_PKT_OVERHEAD       6
> >
> >  #define OLD_SCLK_MIPI_CLK_NAME               "pll_clk"
> >
> > @@ -879,13 +880,40 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
> >                       | DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
> >               samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
> >
> > -             reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> > -                     | DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> > -             samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > +             /*
> > +              * If there is more than one lane, the HFP, HBP, and HSA
> > +              * is calculated in bytes/pixel, then they are divided
> > +              * amongst the different lanes with some additional
> > +              * overhead correction
> > +              */
> > +             if (dsi->lanes > 1) {
> > +                     u32 hfp, hbp, hsa;
> > +                     int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format) / 8;
> > +
> > +                     hfp = ((m->hsync_start - m->hdisplay) * bpp) / dsi->lanes;
> > +                     hfp -= (hfp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> > +
> > +                     hbp = ((m->htotal - m->hsync_end) * bpp) / dsi->lanes;
> > +                     hbp -= (hbp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> >
> > -             reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > -                     | DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> > -             samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > +                     hsa = ((m->hsync_end - m->hsync_start) * bpp) / dsi->lanes;
> > +                     hsa -= (hsa > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> > +
> > +                     reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
> > +                     samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > +
> > +                     reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > +                             | DSIM_MAIN_HSA(hsa);
> > +                     samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > +             } else {
> > +                     reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> > +                             | DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> > +                     samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > +
> > +                     reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > +                             | DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> > +                     samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > +             }
> >       }
> >       reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
> >               DSIM_MAIN_VRESOL(m->vdisplay, num_bits_resol);
>

On Mon, Apr 17, 2023 at 6:53 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 17, 2023 at 3:38 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Hi Adam,
> >
> > Am Samstag, dem 15.04.2023 um 05:41 -0500 schrieb Adam Ford:
> > > NXP uses a lookup table to determine the various values for
> > > the PHY Timing based on the clock rate in their downstream
> > > kernel.  Since the input clock can be variable, the phy
> > > settings need to be variable too.  Add an additional variable
> > > to the driver data to enable this feature to prevent breaking
> > > boards that don't support it.
> > >
> >
> > I haven't checked if this generates values close to the ones in this
> > table, but I guess it should be worth a try to use
> > phy_mipi_dphy_get_default_config() instead.
>
> I didn't know that was a thing.  I like that idea much better than the
> table.  I just pulled what NXP had and tweaked it to fit the mainline.
> I'll give it a try in the next few days, when I have some more time.

I tried phy_mipi_dphy_get_default_config() and the function return
different values than what NXP's table returns, and the screen doesn't
sync properly.  I will try to figure out the mathematical calculation
to generate the values for this DSIM instead of using a lookup table.

adam
>
> adam
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c |  85 +++++++--
> > >  drivers/gpu/drm/bridge/samsung-dsim.h | 254 ++++++++++++++++++++++++++
> > >  include/drm/bridge/samsung-dsim.h     |   1 +
> > >  3 files changed, 326 insertions(+), 14 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.h
> > >
> <snip>
> >

On Mon, Apr 17, 2023 at 2:00 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Montag, 17. April 2023, 00:31:24 CEST schrieb Adam Ford:
> > On Sun, Apr 16, 2023 at 5:07 PM Marek Vasut <marex@denx.de> wrote:
> > > On 4/15/23 12:40, Adam Ford wrote:
> > > > According to Table 13-45 of the i.MX8M Mini Reference Manual, the min
> > > > and max values for M and  the frequency range for the VCO_out
> > > > calculator were incorrect.  This also appears to be the case for the
> > > > imx8mn and imx8mp.
> > > >
> > > > To fix this, make new variables to hold the min and max values of m
> > > > and the minimum value of VCO_out, and update the PMS calculator to
> > > > use these new variables instead of using hard-coded values to keep
> > > > the backwards compatibility with other parts using this driver.
> > >
> > > [...]
> > >
> > > >   static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data =
> > > >   {
> > > >
> > > > @@ -470,6 +485,9 @@ static const struct samsung_dsim_driver_data
> > > > imx8mm_dsi_driver_data = {> >
> > > >        */
> > > >
> > > >       .pll_p_offset = 14,
> > > >       .reg_values = imx8mm_dsim_reg_values,
> > > >
> > > > +     .m_min = 64,
> > > > +     .m_max = 1023,
> > > > +     .vco_min = 1050,
> > >
> > > You might want to call this 'min_freq' since there is a 'max_freq' which
> > > seems to indicate what VCO max frequency is.
> > >
> > > Note that the same datasheet contains the following information:
> > > "
> > > MIPI_DPHY_M_PLLPMS field descriptions
> > >
> > > 12–4 PMS_M
> > > Specifies the PLL PMS value for the M divider
> > > NOTE: The programmable divider range should be within 25 to 125 to
> > > ensure PLL stability.
> >
> > I was confused by this because this statement is not consistent with
> > the link they reference jumps me to the table where it reads M is
> > between 64 and 1023.
> >
> > > NOTE: The M and P divider values should be considered together to ensure
> > > VCO ouput frequency
> > > (VCO_out) range is between 350 MHz to 750 MHz.
> > > Please refer to the topic DPHY PLL for more information.
> >
> > I was confused by this too, because the NXP documentation reads the
> > 350 - 750MHz that you state, but  "Table 13-45: DPHY PLL Parameters"
> > which immediately follows that sentence  on page 4158 shows VCO_out is
> > between 1050-2100 MHz.
> >
> > I compared the PMS values for a variety of frequencies to those that
> > were set in the downstream NXP code, and the PMS values matched.
> > Maybe someone from NXP can explain the discrepancy.
>
> Also note that, according to Table 13-28 in RM (Rev 2 07/2022) for i.MX8M
> Nano, VCO_out and Fout has a maximum of 1500MHz only. Although the note above
> mentions a range  of 1050-2100MHz...

I looked up the limits in NXP's downstream kernel [1] , and I believe
these values match the table in the reference manual instead of the
conflicting sentence.  I am guessing this is why the PMS values I get
match those of the NXP downstream kernel.

adam

[1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/imx/sec_mipi_pll_1432x.h#L38

>
> Best regards,
> Alexander
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

On Thu, Apr 20, 2023 at 8:06 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Adam,
>
> Am Mittwoch, dem 19.04.2023 um 05:47 -0500 schrieb Adam Ford:
> > On Mon, Apr 17, 2023 at 6:55 AM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > >
> > > > Hi Adam,
> > > >
> > > > Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> > > > > If there is more than one lane, the HFP, HBP, and HSA is calculated in
> > > > > bytes/pixel, then they are divided amongst the different lanes with some
> > > > > additional overhead. This is necessary to achieve higher resolutions while
> > > > > keeping the pixel clocks lower as the number of lanes increase.
> > > > >
> > > >
> > > > In the testing I did to come up with my patch "drm: bridge: samsung-
> > > > dsim: fix blanking packet size calculation" the number of lanes didn't
> > > > make any difference. My testing might be flawed, as I could only
> > > > measure the blanking after translation from MIPI DSI to DPI, so I'm
> > > > interested to know what others did here. How did you validate the
> > > > blanking with your patch? Would you have a chance to test my patch and
> > > > see if it works or breaks in your setup?
> >
> > Lucas,
> >
> > I tried your patch instead of mine.  Yours is dependent on the
> > hs_clock being always set to the burst clock which is configured by
> > the device tree.  I unrolled a bit of my stuff and replaced it with
> > yours.  It worked at 1080p, but when I tried a few other resolutions,
> > they did not work.  I assume it's because the DSI clock is fixed and
> > not changing based on the pixel clock.  In the version I did, I only
> > did that math when the lanes were > 1. In your patch, you divide by 8,
> > and in mine, I fetch the bits-per-pixel (which is 8) and I divide by
> > that just in case the bpp ever changes from 8.  Overall,  I think our
> > patches basically do the same thing.
>
> The calculations in your and my patch are quite different. I'm not
> taking into account the number of lanes or the MIPI format. I'm basing

I was looking more at the division by 8 and the overhead correction of 6.
This number 6 also appears in the NXP downstream kernel [1].  I know
Marek V had some concerns about that.

> the blanking size purely on the ratio between MIPI DSI byte clock and
> the DPI interface clock. It's quite counter-intuitive that the host
> would scale the blanking to the number of lanes automatically, but
> still require the MIPI packet offset removed, but that's what my
> measurements showed to produce the correct blanking after translation
> to DPI by the TC358767 bridge chip.

How many lanes is your DSI interface using?

>
> If you dynamically scale the HS clock, then you would need to input the
> real used HS clock to the calculation in my patch, instead of the fixed
> burst mode rate.

I think what you're saying makes sense.

The code I originally modeled this from was from the NXP downstream
kernel where they define the calculation as being in words [2]. I am
not saying the NXP code is perfect, but the NXP code works.  With this
series, my monitors are able to sync a bunch of different resolutions
from 1080p down to 640x480 and a bunch in between with various refresh
rates too. That was the goal of this series.

Instead of just using your patch as-is, I will adapt yours to use the
scaled clock to see how it behaves and get back to you.  I have
finished reworking the dynamic DPHY settings, and I've fixed up making
the PLL device tree optional. If your patch works, I'll drop my
calculation and just build off what you have to use the scaled HS
clock when I submit the V2 and I'll make sure I CC you.

adam

[1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L270
[2] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L914

>
> Regards,
> Lucas

Am Donnerstag, dem 20.04.2023 um 08:24 -0500 schrieb Adam Ford:
> On Thu, Apr 20, 2023 at 8:06 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Hi Adam,
> > 
> > Am Mittwoch, dem 19.04.2023 um 05:47 -0500 schrieb Adam Ford:
> > > On Mon, Apr 17, 2023 at 6:55 AM Adam Ford <aford173@gmail.com> wrote:
> > > > 
> > > > On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > 
> > > > > Hi Adam,
> > > > > 
> > > > > Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> > > > > > If there is more than one lane, the HFP, HBP, and HSA is calculated in
> > > > > > bytes/pixel, then they are divided amongst the different lanes with some
> > > > > > additional overhead. This is necessary to achieve higher resolutions while
> > > > > > keeping the pixel clocks lower as the number of lanes increase.
> > > > > > 
> > > > > 
> > > > > In the testing I did to come up with my patch "drm: bridge: samsung-
> > > > > dsim: fix blanking packet size calculation" the number of lanes didn't
> > > > > make any difference. My testing might be flawed, as I could only
> > > > > measure the blanking after translation from MIPI DSI to DPI, so I'm
> > > > > interested to know what others did here. How did you validate the
> > > > > blanking with your patch? Would you have a chance to test my patch and
> > > > > see if it works or breaks in your setup?
> > > 
> > > Lucas,
> > > 
> > > I tried your patch instead of mine.  Yours is dependent on the
> > > hs_clock being always set to the burst clock which is configured by
> > > the device tree.  I unrolled a bit of my stuff and replaced it with
> > > yours.  It worked at 1080p, but when I tried a few other resolutions,
> > > they did not work.  I assume it's because the DSI clock is fixed and
> > > not changing based on the pixel clock.  In the version I did, I only
> > > did that math when the lanes were > 1. In your patch, you divide by 8,
> > > and in mine, I fetch the bits-per-pixel (which is 8) and I divide by
> > > that just in case the bpp ever changes from 8.  Overall,  I think our
> > > patches basically do the same thing.
> > 
> > The calculations in your and my patch are quite different. I'm not
> > taking into account the number of lanes or the MIPI format. I'm basing
> 
> I was looking more at the division by 8 and the overhead correction of 6.
> This number 6 also appears in the NXP downstream kernel [1].  I know
> Marek V had some concerns about that.
> 
Yea, I don't fully remember the details about the overhead. Need to
page that back in. The division by 8 in my patch is just to get from
the bit to a byte clock, nothing to do with the MIPI format bits per
channel or something like that.

> > the blanking size purely on the ratio between MIPI DSI byte clock and
> > the DPI interface clock. It's quite counter-intuitive that the host
> > would scale the blanking to the number of lanes automatically, but
> > still require the MIPI packet offset removed, but that's what my
> > measurements showed to produce the correct blanking after translation
> > to DPI by the TC358767 bridge chip.
> 
> How many lanes is your DSI interface using?
> 
When I did the measurements to come up with the patch, I varied the
number of lanes between 1 and 4. Different number of lanes didn't make
a difference. In fact trying to compensate for the number of lanes when
calculating the blanking size to program into the controller lead to
wildly wrong blanking on the DPI side of the external bridge.

> > 
> > If you dynamically scale the HS clock, then you would need to input the
> > real used HS clock to the calculation in my patch, instead of the fixed
> > burst mode rate.
> 
> I think what you're saying makes sense.
> 
> The code I originally modeled this from was from the NXP downstream
> kernel where they define the calculation as being in words [2]. I am
> not saying the NXP code is perfect, but the NXP code works.  With this
> series, my monitors are able to sync a bunch of different resolutions
> from 1080p down to 640x480 and a bunch in between with various refresh
> rates too. That was the goal of this series.
> 
> Instead of just using your patch as-is, I will adapt yours to use the
> scaled clock to see how it behaves and get back to you.
> 

Thanks, that would be very much appreciated.

I also don't assert that my calculation is perfect, as I also don't
have any more information and needed to resort to observing the
blanking after translation by the external bridge, so I hope we could
get some better understanding of how things really work by checking
what works for both our systems.

>   I have
> finished reworking the dynamic DPHY settings, and I've fixed up making
> the PLL device tree optional. If your patch works, I'll drop my
> calculation and just build off what you have to use the scaled HS
> clock when I submit the V2 and I'll make sure I CC you.
> 
Thanks, I'm open to re-do my measurements with your new patches.

Regards,
Lucas

> adam
> 
> [1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L270
> [2] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L914
> 
> > 
> > Regards,
> > Lucas

On Thu, Apr 20, 2023 at 8:43 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Donnerstag, dem 20.04.2023 um 08:24 -0500 schrieb Adam Ford:
> > On Thu, Apr 20, 2023 at 8:06 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Hi Adam,
> > >
> > > Am Mittwoch, dem 19.04.2023 um 05:47 -0500 schrieb Adam Ford:
> > > > On Mon, Apr 17, 2023 at 6:55 AM Adam Ford <aford173@gmail.com> wrote:
> > > > >
> > > > > On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > >
> > > > > > Hi Adam,
> > > > > >
> > > > > > Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> > > > > > > If there is more than one lane, the HFP, HBP, and HSA is calculated in
> > > > > > > bytes/pixel, then they are divided amongst the different lanes with some
> > > > > > > additional overhead. This is necessary to achieve higher resolutions while
> > > > > > > keeping the pixel clocks lower as the number of lanes increase.
> > > > > > >
> > > > > >
> > > > > > In the testing I did to come up with my patch "drm: bridge: samsung-
> > > > > > dsim: fix blanking packet size calculation" the number of lanes didn't
> > > > > > make any difference. My testing might be flawed, as I could only
> > > > > > measure the blanking after translation from MIPI DSI to DPI, so I'm
> > > > > > interested to know what others did here. How did you validate the
> > > > > > blanking with your patch? Would you have a chance to test my patch and
> > > > > > see if it works or breaks in your setup?
> > > >
> > > > Lucas,
> > > >
> > > > I tried your patch instead of mine.  Yours is dependent on the
> > > > hs_clock being always set to the burst clock which is configured by
> > > > the device tree.  I unrolled a bit of my stuff and replaced it with
> > > > yours.  It worked at 1080p, but when I tried a few other resolutions,
> > > > they did not work.  I assume it's because the DSI clock is fixed and
> > > > not changing based on the pixel clock.  In the version I did, I only
> > > > did that math when the lanes were > 1. In your patch, you divide by 8,
> > > > and in mine, I fetch the bits-per-pixel (which is 8) and I divide by
> > > > that just in case the bpp ever changes from 8.  Overall,  I think our
> > > > patches basically do the same thing.
> > >
> > > The calculations in your and my patch are quite different. I'm not
> > > taking into account the number of lanes or the MIPI format. I'm basing

I was taking the number of lanes into account in order to calculate
the clock rate, since 4-lanes can run slower.

> >
> > I was looking more at the division by 8 and the overhead correction of 6.
> > This number 6 also appears in the NXP downstream kernel [1].  I know
> > Marek V had some concerns about that.
> >
> Yea, I don't fully remember the details about the overhead. Need to
> page that back in. The division by 8 in my patch is just to get from
> the bit to a byte clock, nothing to do with the MIPI format bits per
> channel or something like that.
>
> > > the blanking size purely on the ratio between MIPI DSI byte clock and
> > > the DPI interface clock. It's quite counter-intuitive that the host
> > > would scale the blanking to the number of lanes automatically, but
> > > still require the MIPI packet offset removed, but that's what my
> > > measurements showed to produce the correct blanking after translation
> > > to DPI by the TC358767 bridge chip.
> >
> > How many lanes is your DSI interface using?
> >
> When I did the measurements to come up with the patch, I varied the
> number of lanes between 1 and 4. Different number of lanes didn't make
> a difference. In fact trying to compensate for the number of lanes when
> calculating the blanking size to program into the controller lead to
> wildly wrong blanking on the DPI side of the external bridge.
>
> > >
> > > If you dynamically scale the HS clock, then you would need to input the
> > > real used HS clock to the calculation in my patch, instead of the fixed
> > > burst mode rate.
> >
> > I think what you're saying makes sense.
> >
> > The code I originally modeled this from was from the NXP downstream
> > kernel where they define the calculation as being in words [2]. I am
> > not saying the NXP code is perfect, but the NXP code works.  With this
> > series, my monitors are able to sync a bunch of different resolutions
> > from 1080p down to 640x480 and a bunch in between with various refresh
> > rates too. That was the goal of this series.
> >
> > Instead of just using your patch as-is, I will adapt yours to use the
> > scaled clock to see how it behaves and get back to you.
> >
>
> Thanks, that would be very much appreciated.

Lucas,

I took your patch and added a dsi state variable named "hs_clock"  to
keep track of the output of samsung_dsim_set_pll which should be the
active high-speed clock.

I then replaced one line in your code to reference the hs_clock
instead of the burst clock:

@@ -960,7 +962,7 @@ static void samsung_dsim_set_display_mode(struct
samsung_dsim *dsi)
        u32 reg;

        if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
-               int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
+               int byte_clk_khz = dsi->hs_clock / 1000 / 8;
                int hfp = (m->hsync_start - m->hdisplay) *
byte_clk_khz / m->clock;

With that change, your patch works with the rest of my code, and I
think it's easier to read, and it doesn't involve recalculating the
clock speed each time since it's cached.  If you're OK with that, I'll
incorporate your code into V2 of my series, and I'll apply my changes
as a subsequent patch.  I hope to be able to send out V2 this weekend.

I would be curious to know frm Marek Szyprowski what the impact is on
the Samsung devices, if any.

adam

>
> I also don't assert that my calculation is perfect, as I also don't
> have any more information and needed to resort to observing the
> blanking after translation by the external bridge, so I hope we could
> get some better understanding of how things really work by checking
> what works for both our systems.
>
> >   I have
> > finished reworking the dynamic DPHY settings, and I've fixed up making
> > the PLL device tree optional. If your patch works, I'll drop my
> > calculation and just build off what you have to use the scaled HS
> > clock when I submit the V2 and I'll make sure I CC you.
> >
> Thanks, I'm open to re-do my measurements with your new patches.
>
> Regards,
> Lucas
>
> > adam
> >
> > [1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L270
> > [2] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/bridge/sec-dsim.c#L914
> >
> > >
> > > Regards,
> > > Lucas
>