The DPHY timings are currently hard coded. Since the input
clock can be variable, the phy timings need to be variable
too. To facilitate this, we need to cache the hs_clock
based on what is generated from the PLL.
The phy_mipi_dphy_get_default_config_for_hsclk function
configures the DPHY timings in pico-seconds, and a small macro
converts those timings into clock cycles based on the hs_clk.
Signed-off-by: Adam Ford <[email protected]>
Signed-off-by: Lucas Stach <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
Tested-by: Frieder Schrempf <[email protected]>
Reviewed-by: Frieder Schrempf <[email protected]>
Tested-by: Michael Walle <[email protected]>
---
drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 08266303c261..3944b7cfbbdf 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",
@@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
} while ((reg & DSIM_PLL_STABLE) == 0);
+ dsi->hs_clock = fout;
+
return fout;
}
@@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
if (driver_data->has_freqband)
return;
+ phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
+ 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, byte_clock);
+ hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
+ clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
+ clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
+ clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
+ clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
+ hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
+ hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
+ hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
+
/* 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 +749,9 @@ 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];
+
+ reg = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
+
samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
/*
@@ -728,10 +767,11 @@ 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];
+
+ 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);
samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
@@ -744,8 +784,11 @@ 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];
+
+ reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
+ DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
+ DSIM_PHYTIMING2_HS_TRAIL(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..d9d431e3b65a 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -93,6 +93,7 @@ struct samsung_dsim {
u32 pll_clk_rate;
u32 burst_clk_rate;
+ u32 hs_clock;
u32 esc_clk_rate;
u32 lanes;
u32 mode_flags;
--
2.39.2
On Mon, May 15, 2023 at 6:57 PM Adam Ford <[email protected]> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too. To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <[email protected]>
> Signed-off-by: Lucas Stach <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Michael Walle <[email protected]>
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 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",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> } while ((reg & DSIM_PLL_STABLE) == 0);
>
> + dsi->hs_clock = fout;
> +
> return fout;
> }
>
> @@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
>
> if (driver_data->has_freqband)
> return;
>
> + phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> + 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.
> + */
Marek Szyprowski -
I was curious to know if you have any opinion on this TODO note and/or
if you have any stuff you can share about how the values of the
following variables are configured?
> +
> + lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
> + hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> + clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> + clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> + clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> + hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> + hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
> +
These 'work' but they don't exactly match the NXP reference code, but
they're not significantly different. The NXP reference manual doesn't
describe how these registers are set, they only publish the register
and bits used. Since you work for Samsung, I was hoping you might
have inside information to know if this is a reasonable approach.
thanks
adam
> /* 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 +749,9 @@ 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];
> +
> + reg = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +
> samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>
> /*
> @@ -728,10 +767,11 @@ 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];
> +
> + 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);
>
> samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>
> @@ -744,8 +784,11 @@ 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];
> +
> + reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> + DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> + DSIM_PHYTIMING2_HS_TRAIL(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..d9d431e3b65a 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -93,6 +93,7 @@ struct samsung_dsim {
>
> u32 pll_clk_rate;
> u32 burst_clk_rate;
> + u32 hs_clock;
> u32 esc_clk_rate;
> u32 lanes;
> u32 mode_flags;
> --
> 2.39.2
>
On Tue, May 16, 2023 at 5:27 AM Adam Ford <[email protected]> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too. To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <[email protected]>
> Signed-off-by: Lucas Stach <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Michael Walle <[email protected]>
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 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",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> } while ((reg & DSIM_PLL_STABLE) == 0);
>
> + dsi->hs_clock = fout;
> +
> return fout;
> }
>
> @@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
>
> if (driver_data->has_freqband)
> return;
>
> + phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> + dsi->lanes, &cfg);
> +
> + /*
> + * TODO:
> + * The tech reference manual for i.MX8M Mini/Nano/Plus
Does it mean, Applications Processor Reference Manual? better add it
clear reference.
> + * 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, byte_clock);
> + hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> + clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> + clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> + clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> + hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> + hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
I think we can do some kind of negotiation has done similar in bsp by
taking inputs from bit_clk and PLL_1432X table. Did you try this? we
thought this approach while writing dsim to support dynamic dphy.
Thanks,
Jagan.
On Wed, May 17, 2023 at 6:28 AM Jagan Teki <[email protected]> wrote:
>
> On Tue, May 16, 2023 at 5:27 AM Adam Ford <[email protected]> wrote:
> >
> > The DPHY timings are currently hard coded. Since the input
> > clock can be variable, the phy timings need to be variable
> > too. To facilitate this, we need to cache the hs_clock
> > based on what is generated from the PLL.
> >
> > The phy_mipi_dphy_get_default_config_for_hsclk function
> > configures the DPHY timings in pico-seconds, and a small macro
> > converts those timings into clock cycles based on the hs_clk.
> >
> > Signed-off-by: Adam Ford <[email protected]>
> > Signed-off-by: Lucas Stach <[email protected]>
> > Tested-by: Chen-Yu Tsai <[email protected]>
> > Tested-by: Frieder Schrempf <[email protected]>
> > Reviewed-by: Frieder Schrempf <[email protected]>
> > Tested-by: Michael Walle <[email protected]>
> > ---
> > drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
> > include/drm/bridge/samsung-dsim.h | 1 +
> > 2 files changed, 51 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 08266303c261..3944b7cfbbdf 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",
> > @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> > reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> > } while ((reg & DSIM_PLL_STABLE) == 0);
> >
> > + dsi->hs_clock = fout;
> > +
> > return fout;
> > }
> >
> > @@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
> >
> > if (driver_data->has_freqband)
> > return;
> >
> > + phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> > + dsi->lanes, &cfg);
> > +
> > + /*
> > + * TODO:
> > + * The tech reference manual for i.MX8M Mini/Nano/Plus
>
> Does it mean, Applications Processor Reference Manual? better add it
> clear reference.
I can do that.
>
> > + * 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, byte_clock);
> > + hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> > + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> > + clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> > + clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> > + clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> > + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> > + hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> > + hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
>
> I think we can do some kind of negotiation has done similar in bsp by
> taking inputs from bit_clk and PLL_1432X table. Did you try this? we
> thought this approach while writing dsim to support dynamic dphy.
I originally attempted to implement the lookup table that was used in
the downstream NXP kernel, but I was told to not use a lookup table
but to calculate them directly instead. I reached out to my NXP rep
and I was told that they could not divulge the contents of these
registers since the DSI driver was under a license from Samsung and
that information was not available outside of an NDA.
When I did the testing for this, I tested a variety of resolutions and
refresh rates and compared the values output from here to those
generated by the NXP lookup table, and they are very close. From what
I could tell, the variance didn't appear to manifest itself on the
monitors that I tried. I tried to explain this in the TODO message,
but maybe it wasn't clear.
Marek S tested this on Exynos and he didn't report any regressions.
adam
>
> Thanks,
> Jagan.
Am Montag, dem 15.05.2023 um 18:57 -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. To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
I'm not going apply a review tag to a patch where I contributed myself,
but FWIW this looks good to me.
> Signed-off-by: Adam Ford <[email protected]>
> Signed-off-by: Lucas Stach <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Michael Walle <[email protected]>
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 08266303c261..3944b7cfbbdf 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",
> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> } while ((reg & DSIM_PLL_STABLE) == 0);
>
> + dsi->hs_clock = fout;
> +
> return fout;
> }
>
> @@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
>
> if (driver_data->has_freqband)
> return;
>
> + phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
> + 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, byte_clock);
> + hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
> + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
> + clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
> + clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
> + clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
> + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
> + hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
> + hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
> +
> /* 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 +749,9 @@ 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];
> +
> + reg = DSIM_PHYTIMING_LPX(lpx) | DSIM_PHYTIMING_HS_EXIT(hs_exit);
> +
> samsung_dsim_write(dsi, DSIM_PHYTIMING_REG, reg);
>
> /*
> @@ -728,10 +767,11 @@ 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];
> +
> + 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);
>
> samsung_dsim_write(dsi, DSIM_PHYTIMING1_REG, reg);
>
> @@ -744,8 +784,11 @@ 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];
> +
> + reg = DSIM_PHYTIMING2_HS_PREPARE(hs_prepare) |
> + DSIM_PHYTIMING2_HS_ZERO(hs_zero) |
> + DSIM_PHYTIMING2_HS_TRAIL(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..d9d431e3b65a 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -93,6 +93,7 @@ struct samsung_dsim {
>
> u32 pll_clk_rate;
> u32 burst_clk_rate;
> + u32 hs_clock;
> u32 esc_clk_rate;
> u32 lanes;
> u32 mode_flags;
Hi Adam,
On 17.05.2023 04:55, Adam Ford wrote:
> On Mon, May 15, 2023 at 6:57 PM Adam Ford <[email protected]> wrote:
>> The DPHY timings are currently hard coded. Since the input
>> clock can be variable, the phy timings need to be variable
>> too. To facilitate this, we need to cache the hs_clock
>> based on what is generated from the PLL.
>>
>> The phy_mipi_dphy_get_default_config_for_hsclk function
>> configures the DPHY timings in pico-seconds, and a small macro
>> converts those timings into clock cycles based on the hs_clk.
>>
>> Signed-off-by: Adam Ford <[email protected]>
>> Signed-off-by: Lucas Stach <[email protected]>
>> Tested-by: Chen-Yu Tsai <[email protected]>
>> Tested-by: Frieder Schrempf <[email protected]>
>> Reviewed-by: Frieder Schrempf <[email protected]>
>> Tested-by: Michael Walle <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/samsung-dsim.c | 57 +++++++++++++++++++++++----
>> include/drm/bridge/samsung-dsim.h | 1 +
>> 2 files changed, 51 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 08266303c261..3944b7cfbbdf 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",
>> @@ -651,6 +653,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>> reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>> } while ((reg & DSIM_PLL_STABLE) == 0);
>>
>> + dsi->hs_clock = fout;
>> +
>> return fout;
>> }
>>
>> @@ -698,13 +702,46 @@ 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 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 byte_clock = dsi->hs_clock / 8;
>>
>> if (driver_data->has_freqband)
>> return;
>>
>> + phy_mipi_dphy_get_default_config_for_hsclk(dsi->hs_clock,
>> + 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.
>> + */
> Marek Szyprowski -
>
> I was curious to know if you have any opinion on this TODO note and/or
> if you have any stuff you can share about how the values of the
> following variables are configured?
>> +
>> + lpx = PS_TO_CYCLE(cfg.lpx, byte_clock);
>> + hs_exit = PS_TO_CYCLE(cfg.hs_exit, byte_clock);
>> + clk_prepare = PS_TO_CYCLE(cfg.clk_prepare, byte_clock);
>> + clk_zero = PS_TO_CYCLE(cfg.clk_zero, byte_clock);
>> + clk_post = PS_TO_CYCLE(cfg.clk_post, byte_clock);
>> + clk_trail = PS_TO_CYCLE(cfg.clk_trail, byte_clock);
>> + hs_prepare = PS_TO_CYCLE(cfg.hs_prepare, byte_clock);
>> + hs_zero = PS_TO_CYCLE(cfg.hs_zero, byte_clock);
>> + hs_trail = PS_TO_CYCLE(cfg.hs_trail, byte_clock);
>> +
> These 'work' but they don't exactly match the NXP reference code, but
> they're not significantly different. The NXP reference manual doesn't
> describe how these registers are set, they only publish the register
> and bits used. Since you work for Samsung, I was hoping you might
> have inside information to know if this is a reasonable approach.
Unfortunately I won't be able to provide any info on that. You may check
the reference Samsung code for various Exynos based products, but I
suspect it will be similar to what was already in the Exynos DSI driver.
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, May 16, 2023 at 5:27 AM Adam Ford <[email protected]> wrote:
>
> The DPHY timings are currently hard coded. Since the input
> clock can be variable, the phy timings need to be variable
> too. To facilitate this, we need to cache the hs_clock
> based on what is generated from the PLL.
>
> The phy_mipi_dphy_get_default_config_for_hsclk function
> configures the DPHY timings in pico-seconds, and a small macro
> converts those timings into clock cycles based on the hs_clk.
>
> Signed-off-by: Adam Ford <[email protected]>
> Signed-off-by: Lucas Stach <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Michael Walle <[email protected]>
> ---
Tested-by: Jagan Teki <[email protected]> # imx8mm-icore