| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3f6be15d-24b2-5608-831c-f15cb7d7cdd2@kontron.de>
Date: Wed, 3 May 2023 17:39:47 +0200
From: Frieder Schrempf <frieder.schrempf@...tron.de>
To: Adam Ford <aford173@...il.com>, dri-devel@...ts.freedesktop.org
Cc: marex@...x.de, aford@...conembedded.com,
Chen-Yu Tsai <wenst@...omium.org>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Inki Dae <inki.dae@...sung.com>,
Jagan Teki <jagan@...rulasolutions.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 5/7] drm: bridge: samsung-dsim: Dynamically configure
DPHY timing
On 02.05.23 03:07, Adam Ford wrote:
> 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 <aford173@...il.com>
> Tested-by: Chen-Yu Tsai <wenst@...omium.org>
A few nitpicks below, otherwise:
Tested on Kontron BL i.MX8MM with SN65DSI84 and ADV7535 bridges.
Tested-by: Frieder Schrempf <frieder.schrempf@...tron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf@...tron.de>
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 79 +++++++++++++++++++++++----
> include/drm/bridge/samsung-dsim.h | 1 +
> 2 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 2dc02a9e37c0..99642230a54a 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -18,9 +18,7 @@
> #include <linux/media-bus-format.h>
> #include <linux/of_device.h>
> #include <linux/phy/phy.h>
> -
> #include <video/mipi_display.h>
> -
Unrelated blank lines removed above!?
> #include <drm/bridge/samsung-dsim.h>
> #include <drm/drm_panel.h>
> #include <drm/drm_print.h>
> @@ -218,6 +216,8 @@
>
> #define OLD_SCLK_MIPI_CLK_NAME "pll_clk"
>
> +#define PS_TO_CYCLE(PS, MHz) DIV64_U64_ROUND_CLOSEST(((PS) * (MHz)), 1000000000000ULL)
Should macro arguments PS and MHz better be all lower-case?
Also, MHz is actually in Hz, right? So it should be renamed.
> +
> static const char *const clk_names[5] = {
> "bus_clk",
> "sclk_mipi",
> @@ -487,6 +487,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 +699,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 +750,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 +770,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 +793,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);
> }
>
> @@ -1337,7 +1395,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
> samsung_dsim_enable_clock(dsi);
> if (driver_data->wait_for_reset)
> samsung_dsim_wait_for_reset(dsi);
> - samsung_dsim_set_phy_ctrl(dsi);
> + if (!driver_data->has_freqband)
samsung_dsim_set_phy_ctrl() already contains a check for
driver_data->has_freqband
> + samsung_dsim_set_phy_ctrl(dsi);
> samsung_dsim_init_link(dsi);
>
> dsi->state |= DSIM_STATE_INITIALIZED;
> 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 {
Powered by blists - more mailing lists