[V5,5/6] drm: bridge: samsung-dsim: Dynamically configure DPHY timing

Commit Message

Adam Ford May 6, 2023, 7:24 p.m. UTC

The DPHY timings are currently hard coded. Since the input
clock can be variable, the phy timings 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.

The phy_mipi_dphy_get_default_config function configures the
DPHY timings in pico-seconds, and a small macro converts those
timings into clock cycles based on the pixel clock rate.

Signed-off-by: Adam Ford <[e-mail]>
Tested-by: Chen-Yu Tsai <[e-mail]>
Tested-by: Frieder Schrempf <[e-mail]>
Reviewed-by: Frieder Schrempf <[e-mail]>
Tested-by: Michael Walle <[e-mail]>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 74 ++++++++++++++++++++++++---
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 68 insertions(+), 7 deletions(-)

Comments

Lucas Stach May 12, 2023, 7:37 p.m. UTC | #1

Hi Adam,

Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings 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.
> 
> The phy_mipi_dphy_get_default_config function configures the
> DPHY timings in pico-seconds, and a small macro converts those
> timings into clock cycles based on the pixel clock rate.
> 
This week I finally had some time to take a deeper look at this series
and test it on some of my systems.

This patch causes issues when the burst clock rate is fixed by
supplying the DT entry. Instead of describing the issue below, I'm
attaching the patch that makes things work on my system.

I would appreciate if you could test this one on your side. Feel free
to squash it into yours if you find it working properly.

Also I would almost bet that dynamic_dphy is working on the Exynos
boards with that fix added. So if anyone with access to those boards
would like to give it a shot, we may be able to get rid of the
hardcoded PHY parameters altogether, which would be a nice cleanup.

Regards,
Lucas

> Signed-off-by: Adam Ford <[e-mail]>
> Tested-by: Chen-Yu Tsai <[e-mail]>
> Tested-by: Frieder Schrempf <[e-mail]>
> Reviewed-by: Frieder Schrempf <[e-mail]>
> Tested-by: Michael Walle <[e-mail]>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 74 ++++++++++++++++++++++++---
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..d19a5c87b749 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -218,6 +218,8 @@
>  
>  #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
>  
> +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> +
>  static const char *const clk_names[5] = {
>  	"bus_clk",
>  	"sclk_mipi",
> @@ -487,6 +489,7 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
>  	.m_min = 64,
>  	.m_max = 1023,
>  	.min_freq = 1050,
> +	.dynamic_dphy = 1,
>  };
>  
>  static const struct samsung_dsim_driver_data *
> @@ -698,13 +701,50 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>  	const unsigned int *reg_values = driver_data->reg_values;
>  	u32 reg;
> +	struct drm_display_mode *m = &dsi->mode;
> +	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	struct phy_configure_opts_mipi_dphy cfg;
> +	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> +	int hs_exit, hs_prepare, hs_zero, hs_trail;
> +	unsigned long long clock_in_hz = m->clock * 1000;
>  
>  	if (driver_data->has_freqband)
>  		return;
>  
> +	/* The dynamic_phy has the ability to adjust PHY Timing settings */
> +	if (driver_data->dynamic_dphy) {
> +		phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes, &cfg);
> +
> +		/*
> +		 * TODO:
> +		 * The tech reference manual for i.MX8M Mini/Nano/Plus
> +		 * doesn't state what the definition of the PHYTIMING
> +		 * bits are beyond their address and bit position.
> +		 * After reviewing NXP's downstream code, it appears
> +		 * that the various PHYTIMING registers take the number
> +		 * of cycles and use various dividers on them.  This
> +		 * calculation does not result in an exact match to the
> +		 * downstream code, but it is very close, and it appears
> +		 * to sync at a variety of resolutions. If someone
> +		 * can get a more accurate mathematical equation needed
> +		 * for these registers, this should be updated.
> +		 */
> +
> +		lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
> +		hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
> +		clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
> +		clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
> +		clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
> +		clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
> +		hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
> +		hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
> +		hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
> +	}
> +
>  	/* B D-PHY: D-PHY Master & Slave Analog Block control */
>  	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
>  		reg_values[PHYCTRL_SLEW_UP];
> +
>  	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
>  
>  	/*
> @@ -712,7 +752,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
>  	 *	burst
>  	 */
> -	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +	if (driver_data->dynamic_dphy)
> +		reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +	else
> +		reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>  
>  	/*
> @@ -728,10 +772,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
>  	 *	the last payload clock bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_CLK_PREPARE] |
> -		reg_values[PHYTIMING_CLK_ZERO] |
> -		reg_values[PHYTIMING_CLK_POST] |
> -		reg_values[PHYTIMING_CLK_TRAIL];
> +	if (driver_data->dynamic_dphy) {
> +		reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
> +		      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
> +		      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
> +		      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
> +	} else {
> +		reg = reg_values[PHYTIMING_CLK_PREPARE] |
> +		      reg_values[PHYTIMING_CLK_ZERO] |
> +		      reg_values[PHYTIMING_CLK_POST] |
> +		      reg_values[PHYTIMING_CLK_TRAIL];
> +	}
>  
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>  
> @@ -744,8 +795,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
>  	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
>  	 *	state after last payload data bit of a HS transmission burst
>  	 */
> -	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
> -		reg_values[PHYTIMING_HS_TRAIL];
> +
> +	if (driver_data->dynamic_dphy) {
> +		reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> +		      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> +		      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
> +	} else {
> +		reg = reg_values[PHYTIMING_HS_PREPARE] |
> +		      reg_values[PHYTIMING_HS_ZERO] |
> +		      reg_values[PHYTIMING_HS_TRAIL];
> +	}
> +
>  	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
>  }
>  
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index a1a5b2b89a7a..76ea8a1720cc 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
>  	const unsigned int *reg_values;
>  	u16 m_min;
>  	u16 m_max;
> +	bool dynamic_dphy;
>  };
>  
>  struct samsung_dsim_host_ops {

Adam Ford May 12, 2023, 8 p.m. UTC | #2

On Fri, May 12, 2023 at 2:37 PM Lucas Stach <[e-mail]> wrote:
>
> Hi Adam,
>
> Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
> > The DPHY timings are currently hard coded. Since the input
> > clock can be variable, the phy timings 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.
> >
> > The phy_mipi_dphy_get_default_config function configures the
> > DPHY timings in pico-seconds, and a small macro converts those
> > timings into clock cycles based on the pixel clock rate.
> >
> This week I finally had some time to take a deeper look at this series
> and test it on some of my systems.

Thanks for testing this!
>
> This patch causes issues when the burst clock rate is fixed by
> supplying the DT entry. Instead of describing the issue below, I'm
> attaching the patch that makes things work on my system.

Oops, sorry about that.

>
> I would appreciate if you could test this one on your side. Feel free
> to squash it into yours if you find it working properly.

I reviewed your patch, and it looks like it makes a lot of sense.
If it works, I'll squash them together and add your name to the sign-off.

>
> Also I would almost bet that dynamic_dphy is working on the Exynos
> boards with that fix added. So if anyone with access to those boards
> would like to give it a shot, we may be able to get rid of the
> hardcoded PHY parameters altogether, which would be a nice cleanup.

I wondered the same thing, but I didn't want to create more work for
Marek S and since there was so much churn getting the original driver
ported, I thought it would be the safest thing to try to give the
imx8m m/n/p the features without breaking the Exynos.

Marek S - Do you want me to post this file without the extra checks to
see if it still works with Exynos?

adam
>
> Regards,
> Lucas
>
> > Signed-off-by: Adam Ford <[e-mail]>
> > Tested-by: Chen-Yu Tsai <[e-mail]>
> > Tested-by: Frieder Schrempf <[e-mail]>
> > Reviewed-by: Frieder Schrempf <[e-mail]>
> > Tested-by: Michael Walle <[e-mail]>
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 74 ++++++++++++++++++++++++---
> >  include/drm/bridge/samsung-dsim.h     |  1 +
> >  2 files changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 08266303c261..d19a5c87b749 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -218,6 +218,8 @@
> >
> >  #define OLD_SCLK_MIPI_CLK_NAME               "pll_clk"
> >
> > +#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
> > +
> >  static const char *const clk_names[5] = {
> >       "bus_clk",
> >       "sclk_mipi",
> > @@ -487,6 +489,7 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> >       .m_min = 64,
> >       .m_max = 1023,
> >       .min_freq = 1050,
> > +     .dynamic_dphy = 1,
> >  };
> >
> >  static const struct samsung_dsim_driver_data *
> > @@ -698,13 +701,50 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
> >       const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> >       const unsigned int *reg_values = driver_data->reg_values;
> >       u32 reg;
> > +     struct drm_display_mode *m = &dsi->mode;
> > +     int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +     struct phy_configure_opts_mipi_dphy cfg;
> > +     int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
> > +     int hs_exit, hs_prepare, hs_zero, hs_trail;
> > +     unsigned long long clock_in_hz = m->clock * 1000;
> >
> >       if (driver_data->has_freqband)
> >               return;
> >
> > +     /* The dynamic_phy has the ability to adjust PHY Timing settings */
> > +     if (driver_data->dynamic_dphy) {
> > +             phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes, &cfg);
> > +
> > +             /*
> > +              * TODO:
> > +              * The tech reference manual for i.MX8M Mini/Nano/Plus
> > +              * doesn't state what the definition of the PHYTIMING
> > +              * bits are beyond their address and bit position.
> > +              * After reviewing NXP's downstream code, it appears
> > +              * that the various PHYTIMING registers take the number
> > +              * of cycles and use various dividers on them.  This
> > +              * calculation does not result in an exact match to the
> > +              * downstream code, but it is very close, and it appears
> > +              * to sync at a variety of resolutions. If someone
> > +              * can get a more accurate mathematical equation needed
> > +              * for these registers, this should be updated.
> > +              */
> > +
> > +             lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
> > +             hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
> > +             clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
> > +             clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
> > +             clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
> > +             clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
> > +             hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
> > +             hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
> > +             hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
> > +     }
> > +
> >       /* B D-PHY: D-PHY Master & Slave Analog Block control */
> >       reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
> >               reg_values[PHYCTRL_SLEW_UP];
> > +
> >       samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
> >
> >       /*
> > @@ -712,7 +752,11 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
> >        * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
> >        *      burst
> >        */
> > -     reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> > +     if (driver_data->dynamic_dphy)
> > +             reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> > +     else
> > +             reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
> > +
> >       samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
> >
> >       /*
> > @@ -728,10 +772,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
> >        * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
> >        *      the last payload clock bit of a HS transmission burst
> >        */
> > -     reg = reg_values[PHYTIMING_CLK_PREPARE] |
> > -             reg_values[PHYTIMING_CLK_ZERO] |
> > -             reg_values[PHYTIMING_CLK_POST] |
> > -             reg_values[PHYTIMING_CLK_TRAIL];
> > +     if (driver_data->dynamic_dphy) {
> > +             reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)  |
> > +                   DSIM_PHYTIMING1_CLK_ZERO(clk_zero)        |
> > +                   DSIM_PHYTIMING1_CLK_POST(clk_post)        |
> > +                   DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
> > +     } else {
> > +             reg = reg_values[PHYTIMING_CLK_PREPARE] |
> > +                   reg_values[PHYTIMING_CLK_ZERO] |
> > +                   reg_values[PHYTIMING_CLK_POST] |
> > +                   reg_values[PHYTIMING_CLK_TRAIL];
> > +     }
> >
> >       samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
> >
> > @@ -744,8 +795,17 @@ static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
> >        * T HS-TRAIL: Time that the transmitter drives the flipped differential
> >        *      state after last payload data bit of a HS transmission burst
> >        */
> > -     reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
> > -             reg_values[PHYTIMING_HS_TRAIL];
> > +
> > +     if (driver_data->dynamic_dphy) {
> > +             reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> > +                   DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> > +                   DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
> > +     } else {
> > +             reg = reg_values[PHYTIMING_HS_PREPARE] |
> > +                   reg_values[PHYTIMING_HS_ZERO] |
> > +                   reg_values[PHYTIMING_HS_TRAIL];
> > +     }
> > +
> >       samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
> >  }
> >
> > diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> > index a1a5b2b89a7a..76ea8a1720cc 100644
> > --- a/include/drm/bridge/samsung-dsim.h
> > +++ b/include/drm/bridge/samsung-dsim.h
> > @@ -62,6 +62,7 @@ struct samsung_dsim_driver_data {
> >       const unsigned int *reg_values;
> >       u16 m_min;
> >       u16 m_max;
> > +     bool dynamic_dphy;
> >  };
> >
> >  struct samsung_dsim_host_ops {
>

Marek Szyprowski May 12, 2023, 9:02 p.m. UTC | #3

On 12.05.2023 22:00, Adam Ford wrote:
> On Fri, May 12, 2023 at 2:37 PM Lucas Stach <[e-mail]> wrote:
>> Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
>>> The DPHY timings are currently hard coded. Since the input
>>> clock can be variable, the phy timings 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.
>>>
>>> The phy_mipi_dphy_get_default_config function configures the
>>> DPHY timings in pico-seconds, and a small macro converts those
>>> timings into clock cycles based on the pixel clock rate.
>>>
>> This week I finally had some time to take a deeper look at this series
>> and test it on some of my systems.
> Thanks for testing this!
>> This patch causes issues when the burst clock rate is fixed by
>> supplying the DT entry. Instead of describing the issue below, I'm
>> attaching the patch that makes things work on my system.
> Oops, sorry about that.
>
>> I would appreciate if you could test this one on your side. Feel free
>> to squash it into yours if you find it working properly.
> I reviewed your patch, and it looks like it makes a lot of sense.
> If it works, I'll squash them together and add your name to the sign-off.
>
>> Also I would almost bet that dynamic_dphy is working on the Exynos
>> boards with that fix added. So if anyone with access to those boards
>> would like to give it a shot, we may be able to get rid of the
>> hardcoded PHY parameters altogether, which would be a nice cleanup.
> I wondered the same thing, but I didn't want to create more work for
> Marek S and since there was so much churn getting the original driver
> ported, I thought it would be the safest thing to try to give the
> imx8m m/n/p the features without breaking the Exynos.
>
> Marek S - Do you want me to post this file without the extra checks to
> see if it still works with Exynos?

Feel free to send me patches to test or just point to your 
work-in-progress git repo.


Best regards

Adam Ford May 13, 2023, 4:25 a.m. UTC | #4

On Fri, May 12, 2023 at 4:02 PM Marek Szyprowski
<[e-mail]> wrote:
>
> On 12.05.2023 22:00, Adam Ford wrote:
> > On Fri, May 12, 2023 at 2:37 PM Lucas Stach <[e-mail]> wrote:
> >> Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
> >>> The DPHY timings are currently hard coded. Since the input
> >>> clock can be variable, the phy timings 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.
> >>>
> >>> The phy_mipi_dphy_get_default_config function configures the
> >>> DPHY timings in pico-seconds, and a small macro converts those
> >>> timings into clock cycles based on the pixel clock rate.
> >>>
> >> This week I finally had some time to take a deeper look at this series
> >> and test it on some of my systems.
> > Thanks for testing this!
> >> This patch causes issues when the burst clock rate is fixed by
> >> supplying the DT entry. Instead of describing the issue below, I'm
> >> attaching the patch that makes things work on my system.
> > Oops, sorry about that.
> >
> >> I would appreciate if you could test this one on your side. Feel free
> >> to squash it into yours if you find it working properly.
> > I reviewed your patch, and it looks like it makes a lot of sense.
> > If it works, I'll squash them together and add your name to the sign-off.

That worked really well, I'll add it to my WIP directory since Marek S
said he'd test the other proposal of dropping the dynamic phy flag and
corresponding check in favor of pushing everyone to the same code.

> >
> >> Also I would almost bet that dynamic_dphy is working on the Exynos
> >> boards with that fix added. So if anyone with access to those boards
> >> would like to give it a shot, we may be able to get rid of the
> >> hardcoded PHY parameters altogether, which would be a nice cleanup.
> > I wondered the same thing, but I didn't want to create more work for
> > Marek S and since there was so much churn getting the original driver
> > ported, I thought it would be the safest thing to try to give the
> > imx8m m/n/p the features without breaking the Exynos.
> >
> > Marek S - Do you want me to post this file without the extra checks to
> > see if it still works with Exynos?
>
> Feel free to send me patches to test or just point to your
> work-in-progress git repo.

Thanks for testing this, Marek S.  My work-in-progress branch is:

https://github.com/aford173/linux/tree/dsim-updates-wip

Depending on what you find will determine how I modify the next
revision of the code I push, so I very much appreciate your feedback.
Hopefully the suggestion from Lucas will work for your applications
and we can reduce some of the code complexity.

adam
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Marek Szyprowski May 15, 2023, 7:37 a.m. UTC | #5

On 13.05.2023 06:25, Adam Ford wrote:
> On Fri, May 12, 2023 at 4:02 PM Marek Szyprowski
> <[e-mail]> wrote:
>> On 12.05.2023 22:00, Adam Ford wrote:
>>> On Fri, May 12, 2023 at 2:37 PM Lucas Stach <[e-mail]> wrote:
>>>> Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
>>>>> The DPHY timings are currently hard coded. Since the input
>>>>> clock can be variable, the phy timings 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.
>>>>>
>>>>> The phy_mipi_dphy_get_default_config function configures the
>>>>> DPHY timings in pico-seconds, and a small macro converts those
>>>>> timings into clock cycles based on the pixel clock rate.
>>>>>
>>>> This week I finally had some time to take a deeper look at this series
>>>> and test it on some of my systems.
>>> Thanks for testing this!
>>>> This patch causes issues when the burst clock rate is fixed by
>>>> supplying the DT entry. Instead of describing the issue below, I'm
>>>> attaching the patch that makes things work on my system.
>>> Oops, sorry about that.
>>>
>>>> I would appreciate if you could test this one on your side. Feel free
>>>> to squash it into yours if you find it working properly.
>>> I reviewed your patch, and it looks like it makes a lot of sense.
>>> If it works, I'll squash them together and add your name to the sign-off.
> That worked really well, I'll add it to my WIP directory since Marek S
> said he'd test the other proposal of dropping the dynamic phy flag and
> corresponding check in favor of pushing everyone to the same code.
>
>>>> Also I would almost bet that dynamic_dphy is working on the Exynos
>>>> boards with that fix added. So if anyone with access to those boards
>>>> would like to give it a shot, we may be able to get rid of the
>>>> hardcoded PHY parameters altogether, which would be a nice cleanup.
>>> I wondered the same thing, but I didn't want to create more work for
>>> Marek S and since there was so much churn getting the original driver
>>> ported, I thought it would be the safest thing to try to give the
>>> imx8m m/n/p the features without breaking the Exynos.
>>>
>>> Marek S - Do you want me to post this file without the extra checks to
>>> see if it still works with Exynos?
>> Feel free to send me patches to test or just point to your
>> work-in-progress git repo.
> Thanks for testing this, Marek S.  My work-in-progress branch is:
>
> https://protect2.fireeye.com/v1/url?k=2eeb1ed9-4e098384-2eea9596-000babd9f1ba-9ad5c339e5ea6e4d&q=1&e=652be603-d622-4d0e-95d3-639656ab1af1&u=https%3A%2F%2Fgithub.com%2Faford173%2Flinux%2Ftree%2Fdsim-updates-wip
>
> Depending on what you find will determine how I modify the next
> revision of the code I push, so I very much appreciate your feedback.
> Hopefully the suggestion from Lucas will work for your applications
> and we can reduce some of the code complexity.

The above mentioned 'dsim-updates-wip' branch works fine on all my 
Exynos based boards.


Best regards

Adam Ford May 15, 2023, 10:25 a.m. UTC | #6

On Mon, May 15, 2023 at 2:37 AM Marek Szyprowski
<[e-mail]> wrote:
>
> On 13.05.2023 06:25, Adam Ford wrote:
> > On Fri, May 12, 2023 at 4:02 PM Marek Szyprowski
> > <[e-mail]> wrote:
> >> On 12.05.2023 22:00, Adam Ford wrote:
> >>> On Fri, May 12, 2023 at 2:37 PM Lucas Stach <[e-mail]> wrote:
> >>>> Am Samstag, dem 06.05.2023 um 14:24 -0500 schrieb Adam Ford:
> >>>>> The DPHY timings are currently hard coded. Since the input
> >>>>> clock can be variable, the phy timings 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.
> >>>>>
> >>>>> The phy_mipi_dphy_get_default_config function configures the
> >>>>> DPHY timings in pico-seconds, and a small macro converts those
> >>>>> timings into clock cycles based on the pixel clock rate.
> >>>>>
> >>>> This week I finally had some time to take a deeper look at this series
> >>>> and test it on some of my systems.
> >>> Thanks for testing this!
> >>>> This patch causes issues when the burst clock rate is fixed by
> >>>> supplying the DT entry. Instead of describing the issue below, I'm
> >>>> attaching the patch that makes things work on my system.
> >>> Oops, sorry about that.
> >>>
> >>>> I would appreciate if you could test this one on your side. Feel free
> >>>> to squash it into yours if you find it working properly.
> >>> I reviewed your patch, and it looks like it makes a lot of sense.
> >>> If it works, I'll squash them together and add your name to the sign-off.
> > That worked really well, I'll add it to my WIP directory since Marek S
> > said he'd test the other proposal of dropping the dynamic phy flag and
> > corresponding check in favor of pushing everyone to the same code.
> >
> >>>> Also I would almost bet that dynamic_dphy is working on the Exynos
> >>>> boards with that fix added. So if anyone with access to those boards
> >>>> would like to give it a shot, we may be able to get rid of the
> >>>> hardcoded PHY parameters altogether, which would be a nice cleanup.
> >>> I wondered the same thing, but I didn't want to create more work for
> >>> Marek S and since there was so much churn getting the original driver
> >>> ported, I thought it would be the safest thing to try to give the
> >>> imx8m m/n/p the features without breaking the Exynos.
> >>>
> >>> Marek S - Do you want me to post this file without the extra checks to
> >>> see if it still works with Exynos?
> >> Feel free to send me patches to test or just point to your
> >> work-in-progress git repo.
> > Thanks for testing this, Marek S.  My work-in-progress branch is:
> >
> > https://protect2.fireeye.com/v1/url?k=2eeb1ed9-4e098384-2eea9596-000babd9f1ba-9ad5c339e5ea6e4d&q=1&e=652be603-d622-4d0e-95d3-639656ab1af1&u=https%3A%2F%2Fgithub.com%2Faford173%2Flinux%2Ftree%2Fdsim-updates-wip
> >
> > Depending on what you find will determine how I modify the next
> > revision of the code I push, so I very much appreciate your feedback.
> > Hopefully the suggestion from Lucas will work for your applications
> > and we can reduce some of the code complexity.
>
> The above mentioned 'dsim-updates-wip' branch works fine on all my
> Exynos based boards.

Thank you for testing.  I'll work on squashing some of the patches
together and eliminating some of the duplicative stuff so the end
result should be the same as what is in WIP and submit another
revision soon.

thanks!

adam
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 08266303c261..d19a5c87b749 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -218,6 +218,8 @@ 
 
 #define OLD_SCLK_MIPI_CLK_NAME		"pll_clk"
 
+#define PS_TO_CYCLE(ps, hz) DIV64_U64_ROUND_CLOSEST(((ps) * (hz)), 1000000000000ULL)
+
 static const char *const clk_names[5] = {
 	"bus_clk",
 	"sclk_mipi",
@@ -487,6 +489,7 @@  static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
 	.m_min = 64,
 	.m_max = 1023,
 	.min_freq = 1050,
+	.dynamic_dphy = 1,
 };
 
 static const struct samsung_dsim_driver_data *
@@ -698,13 +701,50 @@  static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	const unsigned int *reg_values = driver_data->reg_values;
 	u32 reg;
+	struct drm_display_mode *m = &dsi->mode;
+	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	struct phy_configure_opts_mipi_dphy cfg;
+	int clk_prepare, lpx, clk_zero, clk_post, clk_trail;
+	int hs_exit, hs_prepare, hs_zero, hs_trail;
+	unsigned long long clock_in_hz = m->clock * 1000;
 
 	if (driver_data->has_freqband)
 		return;
 
+	/* The dynamic_phy has the ability to adjust PHY Timing settings */
+	if (driver_data->dynamic_dphy) {
+		phy_mipi_dphy_get_default_config(clock_in_hz, bpp, dsi->lanes, &cfg);
+
+		/*
+		 * TODO:
+		 * The tech reference manual for i.MX8M Mini/Nano/Plus
+		 * doesn't state what the definition of the PHYTIMING
+		 * bits are beyond their address and bit position.
+		 * After reviewing NXP's downstream code, it appears
+		 * that the various PHYTIMING registers take the number
+		 * of cycles and use various dividers on them.  This
+		 * calculation does not result in an exact match to the
+		 * downstream code, but it is very close, and it appears
+		 * to sync at a variety of resolutions. If someone
+		 * can get a more accurate mathematical equation needed
+		 * for these registers, this should be updated.
+		 */
+
+		lpx = PS_TO_CYCLE(cfg.lpx, clock_in_hz);
+		hs_exit = PS_TO_CYCLE(cfg.hs_exit, clock_in_hz);
+		clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, clock_in_hz);
+		clk_zero = PS_TO_CYCLE(cfg.clk_zero, clock_in_hz);
+		clk_post = PS_TO_CYCLE(cfg.clk_post, clock_in_hz);
+		clk_trail = PS_TO_CYCLE(cfg.clk_trail, clock_in_hz);
+		hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, clock_in_hz);
+		hs_zero = PS_TO_CYCLE(cfg.hs_zero, clock_in_hz);
+		hs_trail = PS_TO_CYCLE(cfg.hs_trail, clock_in_hz);
+	}
+
 	/* B D-PHY: D-PHY Master & Slave Analog Block control */
 	reg = reg_values[PHYCTRL_ULPS_EXIT] | reg_values[PHYCTRL_VREG_LP] |
 		reg_values[PHYCTRL_SLEW_UP];
+
 	samsung_dsim_write(dsi, DSIM_PHYCTRL_REG, reg);
 
 	/*
@@ -712,7 +752,11 @@  static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
 	 *	burst
 	 */
-	reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
+	if (driver_data->dynamic_dphy)
+		reg  = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
+	else
+		reg = reg_values[PHYTIMING_LPX] | reg_values[PHYTIMING_HS_EXIT];
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
 
 	/*
@@ -728,10 +772,17 @@  static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
 	 *	the last payload clock bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_CLK_PREPARE] |
-		reg_values[PHYTIMING_CLK_ZERO] |
-		reg_values[PHYTIMING_CLK_POST] |
-		reg_values[PHYTIMING_CLK_TRAIL];
+	if (driver_data->dynamic_dphy) {
+		reg = DSIM_PHYTIMING1_CLK_PREPARE(clk_prepare)	|
+		      DSIM_PHYTIMING1_CLK_ZERO(clk_zero)	|
+		      DSIM_PHYTIMING1_CLK_POST(clk_post)	|
+		      DSIM_PHYTIMING1_CLK_TRAIL(clk_trail);
+	} else {
+		reg = reg_values[PHYTIMING_CLK_PREPARE] |
+		      reg_values[PHYTIMING_CLK_ZERO] |
+		      reg_values[PHYTIMING_CLK_POST] |
+		      reg_values[PHYTIMING_CLK_TRAIL];
+	}
 
 	samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
 
@@ -744,8 +795,17 @@  static void samsung_dsim_set_phy_ctrl(struct samsung_dsim *dsi)
 	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
 	 *	state after last payload data bit of a HS transmission burst
 	 */
-	reg = reg_values[PHYTIMING_HS_PREPARE] | reg_values[PHYTIMING_HS_ZERO] |
-		reg_values[PHYTIMING_HS_TRAIL];
+
+	if (driver_data->dynamic_dphy) {
+		reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
+		      DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
+		      DSIM_PHYTIMING2_HS_TRAIL(hs_trail);
+	} else {
+		reg = reg_values[PHYTIMING_HS_PREPARE] |
+		      reg_values[PHYTIMING_HS_ZERO] |
+		      reg_values[PHYTIMING_HS_TRAIL];
+	}
+
 	samsung_dsim_write(dsi, DSIM_PHYTIMING2_REG, reg);
 }
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index a1a5b2b89a7a..76ea8a1720cc 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -62,6 +62,7 @@  struct samsung_dsim_driver_data {
 	const unsigned int *reg_values;
 	u16 m_min;
 	u16 m_max;
+	bool dynamic_dphy;
 };
 
 struct samsung_dsim_host_ops {