[v6,1/8] drm: bridge: dw_mipi_dsi: add initial regmap infrastructure
diff mbox series

Message ID 20200414151955.311949-2-adrian.ratiu@collabora.com
State New
Headers show
Series
  • Genericize DW MIPI DSI bridge and add i.MX 6 driver
Related show

Commit Message

Adrian Ratiu April 14, 2020, 3:19 p.m. UTC
In order to support multiple versions of the Synopsis MIPI DSI host
controller, which have different register layouts but almost identical
HW protocols, we add a regmap infrastructure which can abstract away
register accesses for platform drivers using the bridge.

The controller HW revision is detected during bridge probe which will
be used in future commits to load the relevant register layout which
the bridge will use transparently to the platform drivers.

Suggested-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Adrian Pop <pop.adrian61@gmail.com>
Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
New in v5.
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 208 ++++++++++--------
 1 file changed, 117 insertions(+), 91 deletions(-)

Comments

Enric Balletbo Serra April 15, 2020, 3:53 p.m. UTC | #1
Hi Adrian,

Some few comments/nits below,

Missatge de Adrian Ratiu <adrian.ratiu@collabora.com> del dia dt., 14
d’abr. 2020 a les 17:19:
>
> In order to support multiple versions of the Synopsis MIPI DSI host
> controller, which have different register layouts but almost identical
> HW protocols, we add a regmap infrastructure which can abstract away
> register accesses for platform drivers using the bridge.
>
> The controller HW revision is detected during bridge probe which will
> be used in future commits to load the relevant register layout which
> the bridge will use transparently to the platform drivers.
>
> Suggested-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Adrian Pop <pop.adrian61@gmail.com>
> Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
> New in v5.
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 208 ++++++++++--------
>  1 file changed, 117 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 5ef0f154aa7b..6d9e2f21c9cc 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>

Should Kconfig select REGMAP for this driver?

>  #include <linux/reset.h>
>
>  #include <video/mipi_display.h>
> @@ -227,6 +228,7 @@ struct dw_mipi_dsi {
>         struct drm_bridge *panel_bridge;
>         struct device *dev;
>         void __iomem *base;
> +       struct regmap *regs;
>
>         struct clk *pclk;
>
> @@ -235,6 +237,7 @@ struct dw_mipi_dsi {
>         u32 lanes;
>         u32 format;
>         unsigned long mode_flags;
> +       u32 hw_version;
>
>  #ifdef CONFIG_DEBUG_FS
>         struct dentry *debugfs;
> @@ -249,6 +252,13 @@ struct dw_mipi_dsi {
>         const struct dw_mipi_dsi_plat_data *plat_data;
>  };
>
> +static const struct regmap_config dw_mipi_dsi_regmap_cfg = {
> +       .reg_bits = 32,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .name = "dw-mipi-dsi",
> +};
> +
>  /*
>   * Check if either a link to a master or slave is present
>   */
> @@ -280,16 +290,6 @@ static inline struct dw_mipi_dsi *bridge_to_dsi(struct drm_bridge *bridge)
>         return container_of(bridge, struct dw_mipi_dsi, bridge);
>  }
>
> -static inline void dsi_write(struct dw_mipi_dsi *dsi, u32 reg, u32 val)
> -{
> -       writel(val, dsi->base + reg);
> -}
> -
> -static inline u32 dsi_read(struct dw_mipi_dsi *dsi, u32 reg)
> -{
> -       return readl(dsi->base + reg);
> -}
> -
>  static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>                                    struct mipi_dsi_device *device)
>  {
> @@ -366,29 +366,29 @@ static void dw_mipi_message_config(struct dw_mipi_dsi *dsi,
>         if (lpm)
>                 val |= CMD_MODE_ALL_LP;
>
> -       dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
> -       dsi_write(dsi, DSI_CMD_MODE_CFG, val);
> +       regmap_write(dsi->regs, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
> +       regmap_write(dsi->regs, DSI_CMD_MODE_CFG, val);
>  }
>
>  static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>  {
>         int ret;
> -       u32 val, mask;
> +       u32 val = 0, mask;
>

I think that this change is not needed, `val` is an input variable
that will be overwritten inside the regmap_read_poll_timeout.
Initialize here is not needed.

> -       ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> -                                val, !(val & GEN_CMD_FULL), 1000,
> -                                CMD_PKT_STATUS_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
> +                                      val, !(val & GEN_CMD_FULL), 1000,
> +                                      CMD_PKT_STATUS_TIMEOUT_US);
>         if (ret) {
>                 dev_err(dsi->dev, "failed to get available command FIFO\n");
>                 return ret;
>         }
>
> -       dsi_write(dsi, DSI_GEN_HDR, hdr_val);
> +       regmap_write(dsi->regs, DSI_GEN_HDR, hdr_val);
>
>         mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY;
> -       ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> -                                val, (val & mask) == mask,
> -                                1000, CMD_PKT_STATUS_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
> +                                      val, (val & mask) == mask,
> +                                      1000, CMD_PKT_STATUS_TIMEOUT_US);
>         if (ret) {
>                 dev_err(dsi->dev, "failed to write command FIFO\n");
>                 return ret;
> @@ -403,24 +403,26 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi,
>         const u8 *tx_buf = packet->payload;
>         int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
>         __le32 word;
> -       u32 val;
> +       u32 val = 0;
>

The same here, 'val' is used for the regmap_read_poll_timeout and will
be overwritten, no need to initialize.

>         while (len) {
>                 if (len < pld_data_bytes) {
>                         word = 0;
>                         memcpy(&word, tx_buf, len);
> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word));
> +                       regmap_write(dsi->regs, DSI_GEN_PLD_DATA,
> +                                    le32_to_cpu(word));
>                         len = 0;
>                 } else {
>                         memcpy(&word, tx_buf, pld_data_bytes);
> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word));
> +                       regmap_write(dsi->regs, DSI_GEN_PLD_DATA,
> +                                    le32_to_cpu(word));
>                         tx_buf += pld_data_bytes;
>                         len -= pld_data_bytes;
>                 }
>
> -               ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> -                                        val, !(val & GEN_PLD_W_FULL), 1000,
> -                                        CMD_PKT_STATUS_TIMEOUT_US);
> +               ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
> +                                              val, !(val & GEN_PLD_W_FULL),
> +                                              1000, CMD_PKT_STATUS_TIMEOUT_US);
>                 if (ret) {
>                         dev_err(dsi->dev,
>                                 "failed to get available write payload FIFO\n");
> @@ -438,12 +440,12 @@ static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
>  {
>         int i, j, ret, len = msg->rx_len;
>         u8 *buf = msg->rx_buf;
> -       u32 val;
> +       u32 val = 0;
>

ditto

>         /* Wait end of the read operation */
> -       ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> -                                val, !(val & GEN_RD_CMD_BUSY),
> -                                1000, CMD_PKT_STATUS_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
> +                                      val, !(val & GEN_RD_CMD_BUSY),
> +                                      1000, CMD_PKT_STATUS_TIMEOUT_US);
>         if (ret) {
>                 dev_err(dsi->dev, "Timeout during read operation\n");
>                 return ret;
> @@ -451,15 +453,15 @@ static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
>
>         for (i = 0; i < len; i += 4) {
>                 /* Read fifo must not be empty before all bytes are read */
> -               ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> -                                        val, !(val & GEN_PLD_R_EMPTY),
> -                                        1000, CMD_PKT_STATUS_TIMEOUT_US);
> +               ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
> +                                              val, !(val & GEN_PLD_R_EMPTY),
> +                                              1000, CMD_PKT_STATUS_TIMEOUT_US);
>                 if (ret) {
>                         dev_err(dsi->dev, "Read payload FIFO is empty\n");
>                         return ret;
>                 }
>
> -               val = dsi_read(dsi, DSI_GEN_PLD_DATA);
> +               regmap_read(dsi->regs, DSI_GEN_PLD_DATA, &val);
>                 for (j = 0; j < 4 && j + i < len; j++)
>                         buf[i + j] = val >> (8 * j);
>         }
> @@ -536,29 +538,29 @@ static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
>         }
>  #endif /* CONFIG_DEBUG_FS */
>
> -       dsi_write(dsi, DSI_VID_MODE_CFG, val);
> +       regmap_write(dsi->regs, DSI_VID_MODE_CFG, val);
>  }
>
>  static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi,
>                                  unsigned long mode_flags)
>  {
> -       dsi_write(dsi, DSI_PWR_UP, RESET);
> +       regmap_write(dsi->regs, DSI_PWR_UP, RESET);
>
>         if (mode_flags & MIPI_DSI_MODE_VIDEO) {
> -               dsi_write(dsi, DSI_MODE_CFG, ENABLE_VIDEO_MODE);
> +               regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_VIDEO_MODE);
>                 dw_mipi_dsi_video_mode_config(dsi);
> -               dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
> +               regmap_write(dsi->regs, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
>         } else {
> -               dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
> +               regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE);
>         }
>
> -       dsi_write(dsi, DSI_PWR_UP, POWERUP);
> +       regmap_write(dsi->regs, DSI_PWR_UP, POWERUP);
>  }
>
>  static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi)
>  {
> -       dsi_write(dsi, DSI_PWR_UP, RESET);
> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_RSTZ);
> +       regmap_write(dsi->regs, DSI_PWR_UP, RESET);
> +       regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_RSTZ);
>  }
>
>  static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
> @@ -573,14 +575,14 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
>          */
>         u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1;
>
> -       dsi_write(dsi, DSI_PWR_UP, RESET);
> +       regmap_write(dsi->regs, DSI_PWR_UP, RESET);
>
>         /*
>          * TODO dw drv improvements
>          * timeout clock division should be computed with the
>          * high speed transmission counter timeout and byte lane...
>          */
> -       dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
> +       regmap_write(dsi->regs, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
>                   TX_ESC_CLK_DIVISION(esc_clk_division));
>  }
>
> @@ -609,22 +611,22 @@ static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
>         if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>                 val |= HSYNC_ACTIVE_LOW;
>
> -       dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel));
> -       dsi_write(dsi, DSI_DPI_COLOR_CODING, color);
> -       dsi_write(dsi, DSI_DPI_CFG_POL, val);
> +       regmap_write(dsi->regs, DSI_DPI_VCID, DPI_VCID(dsi->channel));
> +       regmap_write(dsi->regs, DSI_DPI_COLOR_CODING, color);
> +       regmap_write(dsi->regs, DSI_DPI_CFG_POL, val);
>         /*
>          * TODO dw drv improvements
>          * largest packet sizes during hfp or during vsa/vpb/vfp
>          * should be computed according to byte lane, lane number and only
>          * if sending lp cmds in high speed is enable (PHY_TXREQUESTCLKHS)
>          */
> -       dsi_write(dsi, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4)
> +       regmap_write(dsi->regs, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4)
>                   | INVACT_LPCMD_TIME(4));
>  }
>
>  static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
>  {
> -       dsi_write(dsi, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | BTA_EN);
> +       regmap_write(dsi->regs, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | BTA_EN);
>  }
>
>  static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
> @@ -638,7 +640,7 @@ static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
>          * non-burst video modes, see dw_mipi_dsi_video_mode_config()...
>          */
>
> -       dsi_write(dsi, DSI_VID_PKT_SIZE,
> +       regmap_write(dsi->regs, DSI_VID_PKT_SIZE,
>                        dw_mipi_is_dual_mode(dsi) ?
>                                 VID_PKT_SIZE(mode->hdisplay / 2) :
>                                 VID_PKT_SIZE(mode->hdisplay));
> @@ -651,14 +653,15 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
>          * compute high speed transmission counter timeout according
>          * to the timeout clock division (TO_CLK_DIVISION) and byte lane...
>          */
> -       dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
> +       regmap_write(dsi->regs, DSI_TO_CNT_CFG,
> +                    HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
>         /*
>          * TODO dw drv improvements
>          * the Bus-Turn-Around Timeout Counter should be computed
>          * according to byte lane...
>          */
> -       dsi_write(dsi, DSI_BTA_TO_CNT, 0xd00);
> -       dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
> +       regmap_write(dsi->regs, DSI_BTA_TO_CNT, 0xd00);
> +       regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE);
>  }
>
>  /* Get lane byte clock cycles. */
> @@ -692,13 +695,13 @@ static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi,
>          * computations below may be improved...
>          */
>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, htotal);
> -       dsi_write(dsi, DSI_VID_HLINE_TIME, lbcc);
> +       regmap_write(dsi->regs, DSI_VID_HLINE_TIME, lbcc);
>
>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hsa);
> -       dsi_write(dsi, DSI_VID_HSA_TIME, lbcc);
> +       regmap_write(dsi->regs, DSI_VID_HSA_TIME, lbcc);
>
>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hbp);
> -       dsi_write(dsi, DSI_VID_HBP_TIME, lbcc);
> +       regmap_write(dsi->regs, DSI_VID_HBP_TIME, lbcc);
>  }
>
>  static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi,
> @@ -711,10 +714,10 @@ static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi,
>         vfp = mode->vsync_start - mode->vdisplay;
>         vbp = mode->vtotal - mode->vsync_end;
>
> -       dsi_write(dsi, DSI_VID_VACTIVE_LINES, vactive);
> -       dsi_write(dsi, DSI_VID_VSA_LINES, vsa);
> -       dsi_write(dsi, DSI_VID_VFP_LINES, vfp);
> -       dsi_write(dsi, DSI_VID_VBP_LINES, vbp);
> +       regmap_write(dsi->regs, DSI_VID_VACTIVE_LINES, vactive);
> +       regmap_write(dsi->regs, DSI_VID_VSA_LINES, vsa);
> +       regmap_write(dsi->regs, DSI_VID_VFP_LINES, vfp);
> +       regmap_write(dsi->regs, DSI_VID_VBP_LINES, vbp);
>  }
>
>  static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi)
> @@ -737,23 +740,25 @@ static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi)
>          * DSI_CMD_MODE_CFG.MAX_RD_PKT_SIZE_LP (see CMD_MODE_ALL_LP)
>          */
>
> -       hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> +       regmap_read(dsi->regs, DSI_VERSION, &hw_version);
> +       hw_version &= VERSION;

If I am not wrong, you already introduced a function to get the
hardware version and is stored in dsi->hw_version. Maybe you could you
use that variable and remove that code.

>
>         if (hw_version >= HWVER_131) {
> -               dsi_write(dsi, DSI_PHY_TMR_CFG,
> -                         PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
> -                         PHY_LP2HS_TIME_V131(timing.data_lp2hs));
> -               dsi_write(dsi, DSI_PHY_TMR_RD_CFG, MAX_RD_TIME_V131(10000));
> +               regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
> +                            PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
> +                            PHY_LP2HS_TIME_V131(timing.data_lp2hs));
> +               regmap_write(dsi->regs, DSI_PHY_TMR_RD_CFG,
> +                            MAX_RD_TIME_V131(10000));
>         } else {
> -               dsi_write(dsi, DSI_PHY_TMR_CFG,
> -                         PHY_HS2LP_TIME(timing.data_hs2lp) |
> -                         PHY_LP2HS_TIME(timing.data_lp2hs) |
> -                         MAX_RD_TIME(10000));
> +               regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
> +                            PHY_HS2LP_TIME(timing.data_hs2lp) |
> +                            PHY_LP2HS_TIME(timing.data_lp2hs) |
> +                            MAX_RD_TIME(10000));
>         }
>
> -       dsi_write(dsi, DSI_PHY_TMR_LPCLK_CFG,
> -                 PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
> -                 PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
> +       regmap_write(dsi->regs, DSI_PHY_TMR_LPCLK_CFG,
> +                    PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
> +                    PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
>  }
>
>  static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
> @@ -763,46 +768,49 @@ static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
>          * stop wait time should be the maximum between host dsi
>          * and panel stop wait times
>          */
> -       dsi_write(dsi, DSI_PHY_IF_CFG, PHY_STOP_WAIT_TIME(0x20) |
> -                 N_LANES(dsi->lanes));
> +       regmap_write(dsi->regs, DSI_PHY_IF_CFG,
> +                    PHY_STOP_WAIT_TIME(0x20) | N_LANES(dsi->lanes));
>  }
>
>  static void dw_mipi_dsi_dphy_init(struct dw_mipi_dsi *dsi)
>  {
>         /* Clear PHY state */
> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
> -                 | PHY_RSTZ | PHY_SHUTDOWNZ);
> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
> +       regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
> +                    | PHY_RSTZ | PHY_SHUTDOWNZ);
> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
>  }
>
>  static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi *dsi)
>  {
> -       u32 val;
> +       u32 val = 0;

ditto

>         int ret;
>
> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
> -                 PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
> +       regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
> +                    PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
>
> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val,
> -                                val & PHY_LOCK, 1000, PHY_STATUS_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
> +                                      val, val & PHY_LOCK,
> +                                      1000, PHY_STATUS_TIMEOUT_US);
>         if (ret)
>                 DRM_DEBUG_DRIVER("failed to wait phy lock state\n");
>
> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS,
> -                                val, val & PHY_STOP_STATE_CLK_LANE, 1000,
> -                                PHY_STATUS_TIMEOUT_US);
> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
> +                                      val, val & PHY_STOP_STATE_CLK_LANE, 1000,
> +                                      PHY_STATUS_TIMEOUT_US);
>         if (ret)
>                 DRM_DEBUG_DRIVER("failed to wait phy clk lane stop state\n");
>  }
>
>  static void dw_mipi_dsi_clear_err(struct dw_mipi_dsi *dsi)
>  {
> -       dsi_read(dsi, DSI_INT_ST0);
> -       dsi_read(dsi, DSI_INT_ST1);
> -       dsi_write(dsi, DSI_INT_MSK0, 0);
> -       dsi_write(dsi, DSI_INT_MSK1, 0);
> +       u32 val;
> +
> +       regmap_read(dsi->regs, DSI_INT_ST0, &val);
> +       regmap_read(dsi->regs, DSI_INT_ST1, &val);
> +       regmap_write(dsi->regs, DSI_INT_MSK0, 0);
> +       regmap_write(dsi->regs, DSI_INT_MSK1, 0);
>  }
>
>  static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge)
> @@ -989,6 +997,14 @@ static void dw_mipi_dsi_debugfs_remove(struct dw_mipi_dsi *dsi) { }
>
>  #endif /* CONFIG_DEBUG_FS */
>
> +static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi *dsi)
> +{
> +       regmap_read(dsi->regs, DSI_VERSION, &dsi->hw_version);
> +       dsi->hw_version &= VERSION;
> +       if (!dsi->hw_version)
> +               dev_err(dsi->dev, "Failed to read DSI hw version register\n");

Is this an error that should be ignored? If you can't get the HW
version, probably, there is something wrong with your hardware so,
don't you need to return an error?


> +}
> +
>  static struct dw_mipi_dsi *
>  __dw_mipi_dsi_probe(struct platform_device *pdev,
>                     const struct dw_mipi_dsi_plat_data *plat_data)
> @@ -1020,6 +1036,14 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>                 dsi->base = plat_data->base;
>         }
>
> +       dsi->regs = devm_regmap_init_mmio(dev, dsi->base,
> +                                         &dw_mipi_dsi_regmap_cfg);
> +       if (IS_ERR(dsi->regs)) {
> +               ret = PTR_ERR(dsi->regs);
> +               DRM_ERROR("Failed to create DW MIPI DSI regmap: %d\n", ret);
> +               return ERR_PTR(ret);
> +       }
> +
>         dsi->pclk = devm_clk_get(dev, "pclk");
>         if (IS_ERR(dsi->pclk)) {
>                 ret = PTR_ERR(dsi->pclk);
> @@ -1055,6 +1079,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>                 clk_disable_unprepare(dsi->pclk);
>         }
>
> +       dw_mipi_dsi_get_hw_version(dsi);
> +
>         dw_mipi_dsi_debugfs_init(dsi);
>         pm_runtime_enable(dev);
>
> --
> 2.26.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Adrian Ratiu April 16, 2020, 6:25 p.m. UTC | #2
On Wed, 15 Apr 2020, Enric Balletbo Serra <eballetbo@gmail.com> 
wrote:
> Hi Adrian, 
> 
> Some few comments/nits below, 
> 

Hi Enric,

Thank you for your review, all your suggested changes will be part 
of v7. I have only one minor comment below.

> Missatge de Adrian Ratiu <adrian.ratiu@collabora.com> del dia 
> dt., 14 d’abr. 2020 a les 17:19: 
>> 
>> In order to support multiple versions of the Synopsis MIPI DSI 
>> host controller, which have different register layouts but 
>> almost identical HW protocols, we add a regmap infrastructure 
>> which can abstract away register accesses for platform drivers 
>> using the bridge. 
>> 
>> The controller HW revision is detected during bridge probe 
>> which will be used in future commits to load the relevant 
>> register layout which the bridge will use transparently to the 
>> platform drivers. 
>> 
>> Suggested-by: Ezequiel Garcia <ezequiel@collabora.com> 
>> Tested-by: Adrian Pop <pop.adrian61@gmail.com> Tested-by: 
>> Arnaud Ferraris <arnaud.ferraris@collabora.com> Signed-off-by: 
>> Adrian Ratiu <adrian.ratiu@collabora.com> --- New in v5.  --- 
>>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 208 
>>  ++++++++++-------- 1 file changed, 117 insertions(+), 91 
>>  deletions(-) 
>> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 
>> 5ef0f154aa7b..6d9e2f21c9cc 100644 --- 
>> a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ 
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -15,6 +15,7 
>> @@ 
>>  #include <linux/module.h> #include <linux/of_device.h> 
>>  #include <linux/pm_runtime.h> 
>> +#include <linux/regmap.h> 
> 
> Should Kconfig select REGMAP for this driver? 
> 
>>  #include <linux/reset.h> 
>> 
>>  #include <video/mipi_display.h> 
>> @@ -227,6 +228,7 @@ struct dw_mipi_dsi { 
>>         struct drm_bridge *panel_bridge; struct device *dev; 
>>         void __iomem *base; 
>> +       struct regmap *regs; 
>> 
>>         struct clk *pclk; 
>> 
>> @@ -235,6 +237,7 @@ struct dw_mipi_dsi { 
>>         u32 lanes; u32 format; unsigned long mode_flags; 
>> +       u32 hw_version; 
>> 
>>  #ifdef CONFIG_DEBUG_FS 
>>         struct dentry *debugfs; 
>> @@ -249,6 +252,13 @@ struct dw_mipi_dsi { 
>>         const struct dw_mipi_dsi_plat_data *plat_data; 
>>  }; 
>> 
>> +static const struct regmap_config dw_mipi_dsi_regmap_cfg = { + 
>> .reg_bits = 32, +       .val_bits = 32, +       .reg_stride = 
>> 4, +       .name = "dw-mipi-dsi", +}; + 
>>  /* 
>>   * Check if either a link to a master or slave is present */ 
>> @@ -280,16 +290,6 @@ static inline struct dw_mipi_dsi 
>> *bridge_to_dsi(struct drm_bridge *bridge) 
>>         return container_of(bridge, struct dw_mipi_dsi, 
>>         bridge); 
>>  } 
>> 
>> -static inline void dsi_write(struct dw_mipi_dsi *dsi, u32 reg, 
>> u32 val) -{ -       writel(val, dsi->base + reg); -} - -static 
>> inline u32 dsi_read(struct dw_mipi_dsi *dsi, u32 reg) -{ - 
>> return readl(dsi->base + reg); -} - 
>>  static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, 
>>                                    struct mipi_dsi_device 
>>                                    *device) 
>>  { 
>> @@ -366,29 +366,29 @@ static void dw_mipi_message_config(struct 
>> dw_mipi_dsi *dsi, 
>>         if (lpm) 
>>                 val |= CMD_MODE_ALL_LP; 
>> 
>> -       dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : 
>> PHY_TXREQUESTCLKHS); -       dsi_write(dsi, DSI_CMD_MODE_CFG, 
>> val); +       regmap_write(dsi->regs, DSI_LPCLK_CTRL, lpm ? 0 : 
>> PHY_TXREQUESTCLKHS); +       regmap_write(dsi->regs, 
>> DSI_CMD_MODE_CFG, val); 
>>  } 
>> 
>>  static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi 
>>  *dsi, u32 hdr_val) { 
>>         int ret; 
>> -       u32 val, mask; +       u32 val = 0, mask; 
>> 
> 
> I think that this change is not needed, `val` is an input 
> variable that will be overwritten inside the 
> regmap_read_poll_timeout.  Initialize here is not needed. 
> 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, !(val 
>> & GEN_CMD_FULL), 1000, - 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_CMD_FULL), 1000, + 
>> CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "failed to get available 
>>                 command FIFO\n"); return ret; 
>>         } 
>> 
>> -       dsi_write(dsi, DSI_GEN_HDR, hdr_val); + 
>> regmap_write(dsi->regs, DSI_GEN_HDR, hdr_val); 
>> 
>>         mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY; 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, (val 
>> & mask) == mask, -                                1000, 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, (val & mask) == mask, + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "failed to write command 
>>                 FIFO\n"); return ret; 
>> @@ -403,24 +403,26 @@ static int dw_mipi_dsi_write(struct 
>> dw_mipi_dsi *dsi, 
>>         const u8 *tx_buf = packet->payload; int len = 
>>         packet->payload_length, pld_data_bytes = sizeof(u32), 
>>         ret; __le32 word; 
>> -       u32 val; +       u32 val = 0; 
>> 
> 
> The same here, 'val' is used for the regmap_read_poll_timeout 
> and will be overwritten, no need to initialize. 
> 
>>         while (len) { 
>>                 if (len < pld_data_bytes) { 
>>                         word = 0; memcpy(&word, tx_buf, len); 
>> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, 
>> le32_to_cpu(word)); + 
>> regmap_write(dsi->regs, DSI_GEN_PLD_DATA, + 
>> le32_to_cpu(word)); 
>>                         len = 0; 
>>                 } else { 
>>                         memcpy(&word, tx_buf, pld_data_bytes); 
>> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, 
>> le32_to_cpu(word)); + 
>> regmap_write(dsi->regs, DSI_GEN_PLD_DATA, + 
>> le32_to_cpu(word)); 
>>                         tx_buf += pld_data_bytes; len -= 
>>                         pld_data_bytes; 
>>                 } 
>> 
>> -               ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, - 
>> val, !(val & GEN_PLD_W_FULL), 1000, - 
>> CMD_PKT_STATUS_TIMEOUT_US); +               ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_PLD_W_FULL), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>                 if (ret) { 
>>                         dev_err(dsi->dev, 
>>                                 "failed to get available write 
>>                                 payload FIFO\n"); 
>> @@ -438,12 +440,12 @@ static int dw_mipi_dsi_read(struct 
>> dw_mipi_dsi *dsi, 
>>  { 
>>         int i, j, ret, len = msg->rx_len; u8 *buf = 
>>         msg->rx_buf; 
>> -       u32 val; +       u32 val = 0; 
>> 
> 
> ditto 
> 
>>         /* Wait end of the read operation */ 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, !(val 
>> & GEN_RD_CMD_BUSY), -                                1000, 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_RD_CMD_BUSY), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "Timeout during read 
>>                 operation\n"); return ret; 
>> @@ -451,15 +453,15 @@ static int dw_mipi_dsi_read(struct 
>> dw_mipi_dsi *dsi, 
>> 
>>         for (i = 0; i < len; i += 4) { 
>>                 /* Read fifo must not be empty before all bytes 
>>                 are read */ 
>> -               ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, - 
>> val, !(val & GEN_PLD_R_EMPTY), - 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); +               ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_PLD_R_EMPTY), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>                 if (ret) { 
>>                         dev_err(dsi->dev, "Read payload FIFO is 
>>                         empty\n"); return ret; 
>>                 } 
>> 
>> -               val = dsi_read(dsi, DSI_GEN_PLD_DATA); + 
>> regmap_read(dsi->regs, DSI_GEN_PLD_DATA, &val); 
>>                 for (j = 0; j < 4 && j + i < len; j++) 
>>                         buf[i + j] = val >> (8 * j); 
>>         } 
>> @@ -536,29 +538,29 @@ static void 
>> dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi) 
>>         } 
>>  #endif /* CONFIG_DEBUG_FS */ 
>> 
>> -       dsi_write(dsi, DSI_VID_MODE_CFG, val); + 
>> regmap_write(dsi->regs, DSI_VID_MODE_CFG, val); 
>>  } 
>> 
>>  static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi, 
>>                                  unsigned long mode_flags) 
>>  { 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); 
>> 
>>         if (mode_flags & MIPI_DSI_MODE_VIDEO) { 
>> -               dsi_write(dsi, DSI_MODE_CFG, 
>> ENABLE_VIDEO_MODE); +               regmap_write(dsi->regs, 
>> DSI_MODE_CFG, ENABLE_VIDEO_MODE); 
>>                 dw_mipi_dsi_video_mode_config(dsi); 
>> -               dsi_write(dsi, DSI_LPCLK_CTRL, 
>> PHY_TXREQUESTCLKHS); +               regmap_write(dsi->regs, 
>> DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS); 
>>         } else { 
>> -               dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE); 
>> +               regmap_write(dsi->regs, DSI_MODE_CFG, 
>> ENABLE_CMD_MODE); 
>>         } 
>> 
>> -       dsi_write(dsi, DSI_PWR_UP, POWERUP); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, POWERUP); 
>>  } 
>> 
>>  static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi) { 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); - 
>> dsi_write(dsi, DSI_PHY_RSTZ, PHY_RSTZ); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_RSTZ); 
>>  } 
>> 
>>  static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) 
>> @@ -573,14 +575,14 @@ static void dw_mipi_dsi_init(struct 
>> dw_mipi_dsi *dsi) 
>>          */ 
>>         u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1; 
>> 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); 
>> 
>>         /* 
>>          * TODO dw drv improvements * timeout clock division 
>>          should be computed with the * high speed transmission 
>>          counter timeout and byte lane...  */ 
>> -       dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) | + 
>> regmap_write(dsi->regs, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) | 
>>                   TX_ESC_CLK_DIVISION(esc_clk_division)); 
>>  } 
>> 
>> @@ -609,22 +611,22 @@ static void dw_mipi_dsi_dpi_config(struct 
>> dw_mipi_dsi *dsi, 
>>         if (mode->flags & DRM_MODE_FLAG_NHSYNC) 
>>                 val |= HSYNC_ACTIVE_LOW; 
>> 
>> -       dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel)); - 
>> dsi_write(dsi, DSI_DPI_COLOR_CODING, color); - 
>> dsi_write(dsi, DSI_DPI_CFG_POL, val); + 
>> regmap_write(dsi->regs, DSI_DPI_VCID, DPI_VCID(dsi->channel)); 
>> +       regmap_write(dsi->regs, DSI_DPI_COLOR_CODING, color); + 
>> regmap_write(dsi->regs, DSI_DPI_CFG_POL, val); 
>>         /* 
>>          * TODO dw drv improvements * largest packet sizes 
>>          during hfp or during vsa/vpb/vfp * should be computed 
>>          according to byte lane, lane number and only * if 
>>          sending lp cmds in high speed is enable 
>>          (PHY_TXREQUESTCLKHS) */ 
>> -       dsi_write(dsi, DSI_DPI_LP_CMD_TIM, 
>> OUTVACT_LPCMD_TIME(4) +       regmap_write(dsi->regs, 
>> DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4) 
>>                   | INVACT_LPCMD_TIME(4)); 
>>  } 
>> 
>>  static void dw_mipi_dsi_packet_handler_config(struct 
>>  dw_mipi_dsi *dsi) { 
>> -       dsi_write(dsi, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | 
>> BTA_EN); +       regmap_write(dsi->regs, DSI_PCKHDL_CFG, 
>> CRC_RX_EN | ECC_RX_EN | BTA_EN); 
>>  } 
>> 
>>  static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi 
>>  *dsi, 
>> @@ -638,7 +640,7 @@ static void 
>> dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi, 
>>          * non-burst video modes, see 
>>          dw_mipi_dsi_video_mode_config()...  */ 
>> 
>> -       dsi_write(dsi, DSI_VID_PKT_SIZE, + 
>> regmap_write(dsi->regs, DSI_VID_PKT_SIZE, 
>>                        dw_mipi_is_dual_mode(dsi) ? 
>>                                 VID_PKT_SIZE(mode->hdisplay / 
>>                                 2) : 
>>                                 VID_PKT_SIZE(mode->hdisplay)); 
>> @@ -651,14 +653,15 @@ static void 
>> dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) 
>>          * compute high speed transmission counter timeout 
>>          according * to the timeout clock division 
>>          (TO_CLK_DIVISION) and byte lane...  */ 
>> -       dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | 
>> LPRX_TO_CNT(1000)); +       regmap_write(dsi->regs, 
>> DSI_TO_CNT_CFG, +                    HSTX_TO_CNT(1000) | 
>> LPRX_TO_CNT(1000)); 
>>         /* 
>>          * TODO dw drv improvements * the Bus-Turn-Around 
>>          Timeout Counter should be computed * according to byte 
>>          lane...  */ 
>> -       dsi_write(dsi, DSI_BTA_TO_CNT, 0xd00); - 
>> dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE); + 
>> regmap_write(dsi->regs, DSI_BTA_TO_CNT, 0xd00); + 
>> regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE); 
>>  } 
>> 
>>  /* Get lane byte clock cycles. */ 
>> @@ -692,13 +695,13 @@ static void 
>> dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi, 
>>          * computations below may be improved...  */ 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, 
>>         htotal); 
>> -       dsi_write(dsi, DSI_VID_HLINE_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HLINE_TIME, lbcc); 
>> 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hsa); 
>> -       dsi_write(dsi, DSI_VID_HSA_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HSA_TIME, lbcc); 
>> 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hbp); 
>> -       dsi_write(dsi, DSI_VID_HBP_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HBP_TIME, lbcc); 
>>  } 
>> 
>>  static void dw_mipi_dsi_vertical_timing_config(struct 
>>  dw_mipi_dsi *dsi, 
>> @@ -711,10 +714,10 @@ static void 
>> dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi, 
>>         vfp = mode->vsync_start - mode->vdisplay; vbp = 
>>         mode->vtotal - mode->vsync_end; 
>> 
>> -       dsi_write(dsi, DSI_VID_VACTIVE_LINES, vactive); - 
>> dsi_write(dsi, DSI_VID_VSA_LINES, vsa); -       dsi_write(dsi, 
>> DSI_VID_VFP_LINES, vfp); -       dsi_write(dsi, 
>> DSI_VID_VBP_LINES, vbp); +       regmap_write(dsi->regs, 
>> DSI_VID_VACTIVE_LINES, vactive); + 
>> regmap_write(dsi->regs, DSI_VID_VSA_LINES, vsa); + 
>> regmap_write(dsi->regs, DSI_VID_VFP_LINES, vfp); + 
>> regmap_write(dsi->regs, DSI_VID_VBP_LINES, vbp); 
>>  } 
>> 
>>  static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi 
>>  *dsi) 
>> @@ -737,23 +740,25 @@ static void 
>> dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi) 
>>          * DSI_CMD_MODE_CFG.MAX_RD_PKT_SIZE_LP (see 
>>          CMD_MODE_ALL_LP) */ 
>> 
>> -       hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; + 
>> regmap_read(dsi->regs, DSI_VERSION, &hw_version); + 
>> hw_version &= VERSION; 
> 
> If I am not wrong, you already introduced a function to get the 
> hardware version and is stored in dsi->hw_version. Maybe you 
> could you use that variable and remove that code. 
>

Nice observation! There's a reason why I missed this change in the 
past:
in the next patch I remove the version check entirely from this 
function (yet another benefit of using regmap fields for layouts 
heh).

I will do this change because it makes the patch look a bit nicer 
:)

>>
>>         if (hw_version >= HWVER_131) {
>> -               dsi_write(dsi, DSI_PHY_TMR_CFG,
>> -                         PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
>> -                         PHY_LP2HS_TIME_V131(timing.data_lp2hs));
>> -               dsi_write(dsi, DSI_PHY_TMR_RD_CFG, MAX_RD_TIME_V131(10000));
>> +               regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
>> +                            PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
>> +                            PHY_LP2HS_TIME_V131(timing.data_lp2hs));
>> +               regmap_write(dsi->regs, DSI_PHY_TMR_RD_CFG,
>> +                            MAX_RD_TIME_V131(10000));
>>         } else {
>> -               dsi_write(dsi, DSI_PHY_TMR_CFG,
>> -                         PHY_HS2LP_TIME(timing.data_hs2lp) |
>> -                         PHY_LP2HS_TIME(timing.data_lp2hs) |
>> -                         MAX_RD_TIME(10000));
>> +               regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
>> +                            PHY_HS2LP_TIME(timing.data_hs2lp) |
>> +                            PHY_LP2HS_TIME(timing.data_lp2hs) |
>> +                            MAX_RD_TIME(10000));
>>         }
>>
>> -       dsi_write(dsi, DSI_PHY_TMR_LPCLK_CFG,
>> -                 PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
>> -                 PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
>> +       regmap_write(dsi->regs, DSI_PHY_TMR_LPCLK_CFG,
>> +                    PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
>> +                    PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
>>  }
>>
>>  static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
>> @@ -763,46 +768,49 @@ static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
>>          * stop wait time should be the maximum between host dsi
>>          * and panel stop wait times
>>          */
>> -       dsi_write(dsi, DSI_PHY_IF_CFG, PHY_STOP_WAIT_TIME(0x20) |
>> -                 N_LANES(dsi->lanes));
>> +       regmap_write(dsi->regs, DSI_PHY_IF_CFG,
>> +                    PHY_STOP_WAIT_TIME(0x20) | N_LANES(dsi->lanes));
>>  }
>>
>>  static void dw_mipi_dsi_dphy_init(struct dw_mipi_dsi *dsi)
>>  {
>>         /* Clear PHY state */
>> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
>> -                 | PHY_RSTZ | PHY_SHUTDOWNZ);
>> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
>> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
>> -       dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
>> +       regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
>> +                    | PHY_RSTZ | PHY_SHUTDOWNZ);
>> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
>> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
>> +       regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
>>  }
>>
>>  static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi *dsi)
>>  {
>> -       u32 val;
>> +       u32 val = 0;
>
> ditto
>
>>         int ret;
>>
>> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
>> -                 PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
>> +       regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
>> +                    PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
>>
>> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val,
>> -                                val & PHY_LOCK, 1000, PHY_STATUS_TIMEOUT_US);
>> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
>> +                                      val, val & PHY_LOCK,
>> +                                      1000, PHY_STATUS_TIMEOUT_US);
>>         if (ret)
>>                 DRM_DEBUG_DRIVER("failed to wait phy lock state\n");
>>
>> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS,
>> -                                val, val & PHY_STOP_STATE_CLK_LANE, 1000,
>> -                                PHY_STATUS_TIMEOUT_US);
>> +       ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
>> +                                      val, val & PHY_STOP_STATE_CLK_LANE, 1000,
>> +                                      PHY_STATUS_TIMEOUT_US);
>>         if (ret)
>>                 DRM_DEBUG_DRIVER("failed to wait phy clk lane stop state\n");
>>  }
>>
>>  static void dw_mipi_dsi_clear_err(struct dw_mipi_dsi *dsi)
>>  {
>> -       dsi_read(dsi, DSI_INT_ST0);
>> -       dsi_read(dsi, DSI_INT_ST1);
>> -       dsi_write(dsi, DSI_INT_MSK0, 0);
>> -       dsi_write(dsi, DSI_INT_MSK1, 0);
>> +       u32 val;
>> +
>> +       regmap_read(dsi->regs, DSI_INT_ST0, &val);
>> +       regmap_read(dsi->regs, DSI_INT_ST1, &val);
>> +       regmap_write(dsi->regs, DSI_INT_MSK0, 0);
>> +       regmap_write(dsi->regs, DSI_INT_MSK1, 0);
>>  }
>>
>>  static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge)
>> @@ -989,6 +997,14 @@ static void dw_mipi_dsi_debugfs_remove(struct dw_mipi_dsi *dsi) { }
>>
>>  #endif /* CONFIG_DEBUG_FS */
>>
>> +static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi *dsi)
>> +{
>> +       regmap_read(dsi->regs, DSI_VERSION, &dsi->hw_version);
>> +       dsi->hw_version &= VERSION;
>> +       if (!dsi->hw_version)
>> +               dev_err(dsi->dev, "Failed to read DSI hw version register\n");
>
> Is this an error that should be ignored? If you can't get the HW
> version, probably, there is something wrong with your hardware so,
> don't you need to return an error?
>
>
>> +}
>> +
>>  static struct dw_mipi_dsi *
>>  __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                     const struct dw_mipi_dsi_plat_data *plat_data)
>> @@ -1020,6 +1036,14 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                 dsi->base = plat_data->base;
>>         }
>>
>> +       dsi->regs = devm_regmap_init_mmio(dev, dsi->base,
>> +                                         &dw_mipi_dsi_regmap_cfg);
>> +       if (IS_ERR(dsi->regs)) {
>> +               ret = PTR_ERR(dsi->regs);
>> +               DRM_ERROR("Failed to create DW MIPI DSI regmap: %d\n", ret);
>> +               return ERR_PTR(ret);
>> +       }
>> +
>>         dsi->pclk = devm_clk_get(dev, "pclk");
>>         if (IS_ERR(dsi->pclk)) {
>>                 ret = PTR_ERR(dsi->pclk);
>> @@ -1055,6 +1079,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                 clk_disable_unprepare(dsi->pclk);
>>         }
>>
>> +       dw_mipi_dsi_get_hw_version(dsi);
>> +
>>         dw_mipi_dsi_debugfs_init(dsi);
>>         pm_runtime_enable(dev);
>>
>> --
>> 2.26.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Adrian Ratiu April 16, 2020, 6:41 p.m. UTC | #3
On Wed, 15 Apr 2020, Enric Balletbo Serra <eballetbo@gmail.com> 
wrote:
> Hi Adrian, 
> 
> Some few comments/nits below, 
> 
> Missatge de Adrian Ratiu <adrian.ratiu@collabora.com> del dia 
> dt., 14 d’abr. 2020 a les 17:19: 
>> 
>> In order to support multiple versions of the Synopsis MIPI DSI 
>> host controller, which have different register layouts but 
>> almost identical HW protocols, we add a regmap infrastructure 
>> which can abstract away register accesses for platform drivers 
>> using the bridge. 
>> 
>> The controller HW revision is detected during bridge probe 
>> which will be used in future commits to load the relevant 
>> register layout which the bridge will use transparently to the 
>> platform drivers. 
>> 
>> Suggested-by: Ezequiel Garcia <ezequiel@collabora.com> 
>> Tested-by: Adrian Pop <pop.adrian61@gmail.com> Tested-by: 
>> Arnaud Ferraris <arnaud.ferraris@collabora.com> Signed-off-by: 
>> Adrian Ratiu <adrian.ratiu@collabora.com> --- New in v5.  --- 
>>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 208 
>>  ++++++++++-------- 1 file changed, 117 insertions(+), 91 
>>  deletions(-) 
>> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 
>> 5ef0f154aa7b..6d9e2f21c9cc 100644 --- 
>> a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ 
>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -15,6 +15,7 
>> @@ 
>>  #include <linux/module.h> #include <linux/of_device.h> 
>>  #include <linux/pm_runtime.h> 
>> +#include <linux/regmap.h> 
> 
> Should Kconfig select REGMAP for this driver? 
> 
>>  #include <linux/reset.h> 
>> 
>>  #include <video/mipi_display.h> 
>> @@ -227,6 +228,7 @@ struct dw_mipi_dsi { 
>>         struct drm_bridge *panel_bridge; struct device *dev; 
>>         void __iomem *base; 
>> +       struct regmap *regs; 
>> 
>>         struct clk *pclk; 
>> 
>> @@ -235,6 +237,7 @@ struct dw_mipi_dsi { 
>>         u32 lanes; u32 format; unsigned long mode_flags; 
>> +       u32 hw_version; 
>> 
>>  #ifdef CONFIG_DEBUG_FS 
>>         struct dentry *debugfs; 
>> @@ -249,6 +252,13 @@ struct dw_mipi_dsi { 
>>         const struct dw_mipi_dsi_plat_data *plat_data; 
>>  }; 
>> 
>> +static const struct regmap_config dw_mipi_dsi_regmap_cfg = { + 
>> .reg_bits = 32, +       .val_bits = 32, +       .reg_stride = 
>> 4, +       .name = "dw-mipi-dsi", +}; + 
>>  /* 
>>   * Check if either a link to a master or slave is present */ 
>> @@ -280,16 +290,6 @@ static inline struct dw_mipi_dsi 
>> *bridge_to_dsi(struct drm_bridge *bridge) 
>>         return container_of(bridge, struct dw_mipi_dsi, 
>>         bridge); 
>>  } 
>> 
>> -static inline void dsi_write(struct dw_mipi_dsi *dsi, u32 reg, 
>> u32 val) -{ -       writel(val, dsi->base + reg); -} - -static 
>> inline u32 dsi_read(struct dw_mipi_dsi *dsi, u32 reg) -{ - 
>> return readl(dsi->base + reg); -} - 
>>  static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, 
>>                                    struct mipi_dsi_device 
>>                                    *device) 
>>  { 
>> @@ -366,29 +366,29 @@ static void dw_mipi_message_config(struct 
>> dw_mipi_dsi *dsi, 
>>         if (lpm) 
>>                 val |= CMD_MODE_ALL_LP; 
>> 
>> -       dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : 
>> PHY_TXREQUESTCLKHS); -       dsi_write(dsi, DSI_CMD_MODE_CFG, 
>> val); +       regmap_write(dsi->regs, DSI_LPCLK_CTRL, lpm ? 0 : 
>> PHY_TXREQUESTCLKHS); +       regmap_write(dsi->regs, 
>> DSI_CMD_MODE_CFG, val); 
>>  } 
>> 
>>  static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi 
>>  *dsi, u32 hdr_val) { 
>>         int ret; 
>> -       u32 val, mask; +       u32 val = 0, mask; 
>> 
> 
> I think that this change is not needed, `val` is an input 
> variable that will be overwritten inside the 
> regmap_read_poll_timeout.  Initialize here is not needed. 
> 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, !(val 
>> & GEN_CMD_FULL), 1000, - 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_CMD_FULL), 1000, + 
>> CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "failed to get available 
>>                 command FIFO\n"); return ret; 
>>         } 
>> 
>> -       dsi_write(dsi, DSI_GEN_HDR, hdr_val); + 
>> regmap_write(dsi->regs, DSI_GEN_HDR, hdr_val); 
>> 
>>         mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY; 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, (val 
>> & mask) == mask, -                                1000, 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, (val & mask) == mask, + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "failed to write command 
>>                 FIFO\n"); return ret; 
>> @@ -403,24 +403,26 @@ static int dw_mipi_dsi_write(struct 
>> dw_mipi_dsi *dsi, 
>>         const u8 *tx_buf = packet->payload; int len = 
>>         packet->payload_length, pld_data_bytes = sizeof(u32), 
>>         ret; __le32 word; 
>> -       u32 val; +       u32 val = 0; 
>> 
> 
> The same here, 'val' is used for the regmap_read_poll_timeout 
> and will be overwritten, no need to initialize. 
> 
>>         while (len) { 
>>                 if (len < pld_data_bytes) { 
>>                         word = 0; memcpy(&word, tx_buf, len); 
>> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, 
>> le32_to_cpu(word)); + 
>> regmap_write(dsi->regs, DSI_GEN_PLD_DATA, + 
>> le32_to_cpu(word)); 
>>                         len = 0; 
>>                 } else { 
>>                         memcpy(&word, tx_buf, pld_data_bytes); 
>> -                       dsi_write(dsi, DSI_GEN_PLD_DATA, 
>> le32_to_cpu(word)); + 
>> regmap_write(dsi->regs, DSI_GEN_PLD_DATA, + 
>> le32_to_cpu(word)); 
>>                         tx_buf += pld_data_bytes; len -= 
>>                         pld_data_bytes; 
>>                 } 
>> 
>> -               ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, - 
>> val, !(val & GEN_PLD_W_FULL), 1000, - 
>> CMD_PKT_STATUS_TIMEOUT_US); +               ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_PLD_W_FULL), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>                 if (ret) { 
>>                         dev_err(dsi->dev, 
>>                                 "failed to get available write 
>>                                 payload FIFO\n"); 
>> @@ -438,12 +440,12 @@ static int dw_mipi_dsi_read(struct 
>> dw_mipi_dsi *dsi, 
>>  { 
>>         int i, j, ret, len = msg->rx_len; u8 *buf = 
>>         msg->rx_buf; 
>> -       u32 val; +       u32 val = 0; 
>> 
> 
> ditto 
> 
>>         /* Wait end of the read operation */ 
>> -       ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, -                                val, !(val 
>> & GEN_RD_CMD_BUSY), -                                1000, 
>> CMD_PKT_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_RD_CMD_BUSY), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>         if (ret) { 
>>                 dev_err(dsi->dev, "Timeout during read 
>>                 operation\n"); return ret; 
>> @@ -451,15 +453,15 @@ static int dw_mipi_dsi_read(struct 
>> dw_mipi_dsi *dsi, 
>> 
>>         for (i = 0; i < len; i += 4) { 
>>                 /* Read fifo must not be empty before all bytes 
>>                 are read */ 
>> -               ret = readl_poll_timeout(dsi->base + 
>> DSI_CMD_PKT_STATUS, - 
>> val, !(val & GEN_PLD_R_EMPTY), - 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); +               ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS, + 
>> val, !(val & GEN_PLD_R_EMPTY), + 
>> 1000, CMD_PKT_STATUS_TIMEOUT_US); 
>>                 if (ret) { 
>>                         dev_err(dsi->dev, "Read payload FIFO is 
>>                         empty\n"); return ret; 
>>                 } 
>> 
>> -               val = dsi_read(dsi, DSI_GEN_PLD_DATA); + 
>> regmap_read(dsi->regs, DSI_GEN_PLD_DATA, &val); 
>>                 for (j = 0; j < 4 && j + i < len; j++) 
>>                         buf[i + j] = val >> (8 * j); 
>>         } 
>> @@ -536,29 +538,29 @@ static void 
>> dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi) 
>>         } 
>>  #endif /* CONFIG_DEBUG_FS */ 
>> 
>> -       dsi_write(dsi, DSI_VID_MODE_CFG, val); + 
>> regmap_write(dsi->regs, DSI_VID_MODE_CFG, val); 
>>  } 
>> 
>>  static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi, 
>>                                  unsigned long mode_flags) 
>>  { 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); 
>> 
>>         if (mode_flags & MIPI_DSI_MODE_VIDEO) { 
>> -               dsi_write(dsi, DSI_MODE_CFG, 
>> ENABLE_VIDEO_MODE); +               regmap_write(dsi->regs, 
>> DSI_MODE_CFG, ENABLE_VIDEO_MODE); 
>>                 dw_mipi_dsi_video_mode_config(dsi); 
>> -               dsi_write(dsi, DSI_LPCLK_CTRL, 
>> PHY_TXREQUESTCLKHS); +               regmap_write(dsi->regs, 
>> DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS); 
>>         } else { 
>> -               dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE); 
>> +               regmap_write(dsi->regs, DSI_MODE_CFG, 
>> ENABLE_CMD_MODE); 
>>         } 
>> 
>> -       dsi_write(dsi, DSI_PWR_UP, POWERUP); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, POWERUP); 
>>  } 
>> 
>>  static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi) { 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); - 
>> dsi_write(dsi, DSI_PHY_RSTZ, PHY_RSTZ); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_RSTZ); 
>>  } 
>> 
>>  static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) 
>> @@ -573,14 +575,14 @@ static void dw_mipi_dsi_init(struct 
>> dw_mipi_dsi *dsi) 
>>          */ 
>>         u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1; 
>> 
>> -       dsi_write(dsi, DSI_PWR_UP, RESET); + 
>> regmap_write(dsi->regs, DSI_PWR_UP, RESET); 
>> 
>>         /* 
>>          * TODO dw drv improvements * timeout clock division 
>>          should be computed with the * high speed transmission 
>>          counter timeout and byte lane...  */ 
>> -       dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) | + 
>> regmap_write(dsi->regs, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) | 
>>                   TX_ESC_CLK_DIVISION(esc_clk_division)); 
>>  } 
>> 
>> @@ -609,22 +611,22 @@ static void dw_mipi_dsi_dpi_config(struct 
>> dw_mipi_dsi *dsi, 
>>         if (mode->flags & DRM_MODE_FLAG_NHSYNC) 
>>                 val |= HSYNC_ACTIVE_LOW; 
>> 
>> -       dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel)); - 
>> dsi_write(dsi, DSI_DPI_COLOR_CODING, color); - 
>> dsi_write(dsi, DSI_DPI_CFG_POL, val); + 
>> regmap_write(dsi->regs, DSI_DPI_VCID, DPI_VCID(dsi->channel)); 
>> +       regmap_write(dsi->regs, DSI_DPI_COLOR_CODING, color); + 
>> regmap_write(dsi->regs, DSI_DPI_CFG_POL, val); 
>>         /* 
>>          * TODO dw drv improvements * largest packet sizes 
>>          during hfp or during vsa/vpb/vfp * should be computed 
>>          according to byte lane, lane number and only * if 
>>          sending lp cmds in high speed is enable 
>>          (PHY_TXREQUESTCLKHS) */ 
>> -       dsi_write(dsi, DSI_DPI_LP_CMD_TIM, 
>> OUTVACT_LPCMD_TIME(4) +       regmap_write(dsi->regs, 
>> DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4) 
>>                   | INVACT_LPCMD_TIME(4)); 
>>  } 
>> 
>>  static void dw_mipi_dsi_packet_handler_config(struct 
>>  dw_mipi_dsi *dsi) { 
>> -       dsi_write(dsi, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | 
>> BTA_EN); +       regmap_write(dsi->regs, DSI_PCKHDL_CFG, 
>> CRC_RX_EN | ECC_RX_EN | BTA_EN); 
>>  } 
>> 
>>  static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi 
>>  *dsi, 
>> @@ -638,7 +640,7 @@ static void 
>> dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi, 
>>          * non-burst video modes, see 
>>          dw_mipi_dsi_video_mode_config()...  */ 
>> 
>> -       dsi_write(dsi, DSI_VID_PKT_SIZE, + 
>> regmap_write(dsi->regs, DSI_VID_PKT_SIZE, 
>>                        dw_mipi_is_dual_mode(dsi) ? 
>>                                 VID_PKT_SIZE(mode->hdisplay / 
>>                                 2) : 
>>                                 VID_PKT_SIZE(mode->hdisplay)); 
>> @@ -651,14 +653,15 @@ static void 
>> dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) 
>>          * compute high speed transmission counter timeout 
>>          according * to the timeout clock division 
>>          (TO_CLK_DIVISION) and byte lane...  */ 
>> -       dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | 
>> LPRX_TO_CNT(1000)); +       regmap_write(dsi->regs, 
>> DSI_TO_CNT_CFG, +                    HSTX_TO_CNT(1000) | 
>> LPRX_TO_CNT(1000)); 
>>         /* 
>>          * TODO dw drv improvements * the Bus-Turn-Around 
>>          Timeout Counter should be computed * according to byte 
>>          lane...  */ 
>> -       dsi_write(dsi, DSI_BTA_TO_CNT, 0xd00); - 
>> dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE); + 
>> regmap_write(dsi->regs, DSI_BTA_TO_CNT, 0xd00); + 
>> regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE); 
>>  } 
>> 
>>  /* Get lane byte clock cycles. */ 
>> @@ -692,13 +695,13 @@ static void 
>> dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi, 
>>          * computations below may be improved...  */ 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, 
>>         htotal); 
>> -       dsi_write(dsi, DSI_VID_HLINE_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HLINE_TIME, lbcc); 
>> 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hsa); 
>> -       dsi_write(dsi, DSI_VID_HSA_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HSA_TIME, lbcc); 
>> 
>>         lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hbp); 
>> -       dsi_write(dsi, DSI_VID_HBP_TIME, lbcc); + 
>> regmap_write(dsi->regs, DSI_VID_HBP_TIME, lbcc); 
>>  } 
>> 
>>  static void dw_mipi_dsi_vertical_timing_config(struct 
>>  dw_mipi_dsi *dsi, 
>> @@ -711,10 +714,10 @@ static void 
>> dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi, 
>>         vfp = mode->vsync_start - mode->vdisplay; vbp = 
>>         mode->vtotal - mode->vsync_end; 
>> 
>> -       dsi_write(dsi, DSI_VID_VACTIVE_LINES, vactive); - 
>> dsi_write(dsi, DSI_VID_VSA_LINES, vsa); -       dsi_write(dsi, 
>> DSI_VID_VFP_LINES, vfp); -       dsi_write(dsi, 
>> DSI_VID_VBP_LINES, vbp); +       regmap_write(dsi->regs, 
>> DSI_VID_VACTIVE_LINES, vactive); + 
>> regmap_write(dsi->regs, DSI_VID_VSA_LINES, vsa); + 
>> regmap_write(dsi->regs, DSI_VID_VFP_LINES, vfp); + 
>> regmap_write(dsi->regs, DSI_VID_VBP_LINES, vbp); 
>>  } 
>> 
>>  static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi 
>>  *dsi) 
>> @@ -737,23 +740,25 @@ static void 
>> dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi) 
>>          * DSI_CMD_MODE_CFG.MAX_RD_PKT_SIZE_LP (see 
>>          CMD_MODE_ALL_LP) */ 
>> 
>> -       hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; + 
>> regmap_read(dsi->regs, DSI_VERSION, &hw_version); + 
>> hw_version &= VERSION; 
> 
> If I am not wrong, you already introduced a function to get the 
> hardware version and is stored in dsi->hw_version. Maybe you 
> could you use that variable and remove that code. 
> 
>> 
>>         if (hw_version >= HWVER_131) { 
>> -               dsi_write(dsi, DSI_PHY_TMR_CFG, - 
>> PHY_HS2LP_TIME_V131(timing.data_hs2lp) | - 
>> PHY_LP2HS_TIME_V131(timing.data_lp2hs)); - 
>> dsi_write(dsi, DSI_PHY_TMR_RD_CFG, MAX_RD_TIME_V131(10000)); + 
>> regmap_write(dsi->regs, DSI_PHY_TMR_CFG, + 
>> PHY_HS2LP_TIME_V131(timing.data_hs2lp) | + 
>> PHY_LP2HS_TIME_V131(timing.data_lp2hs)); + 
>> regmap_write(dsi->regs, DSI_PHY_TMR_RD_CFG, + 
>> MAX_RD_TIME_V131(10000)); 
>>         } else { 
>> -               dsi_write(dsi, DSI_PHY_TMR_CFG, - 
>> PHY_HS2LP_TIME(timing.data_hs2lp) | - 
>> PHY_LP2HS_TIME(timing.data_lp2hs) | - 
>> MAX_RD_TIME(10000)); +               regmap_write(dsi->regs, 
>> DSI_PHY_TMR_CFG, + 
>> PHY_HS2LP_TIME(timing.data_hs2lp) | + 
>> PHY_LP2HS_TIME(timing.data_lp2hs) | + 
>> MAX_RD_TIME(10000)); 
>>         } 
>> 
>> -       dsi_write(dsi, DSI_PHY_TMR_LPCLK_CFG, - 
>> PHY_CLKHS2LP_TIME(timing.clk_hs2lp) | - 
>> PHY_CLKLP2HS_TIME(timing.clk_lp2hs)); + 
>> regmap_write(dsi->regs, DSI_PHY_TMR_LPCLK_CFG, + 
>> PHY_CLKHS2LP_TIME(timing.clk_hs2lp) | + 
>> PHY_CLKLP2HS_TIME(timing.clk_lp2hs)); 
>>  } 
>> 
>>  static void dw_mipi_dsi_dphy_interface_config(struct 
>>  dw_mipi_dsi *dsi) 
>> @@ -763,46 +768,49 @@ static void 
>> dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi) 
>>          * stop wait time should be the maximum between host 
>>          dsi * and panel stop wait times */ 
>> -       dsi_write(dsi, DSI_PHY_IF_CFG, PHY_STOP_WAIT_TIME(0x20) 
>> | -                 N_LANES(dsi->lanes)); + 
>> regmap_write(dsi->regs, DSI_PHY_IF_CFG, + 
>> PHY_STOP_WAIT_TIME(0x20) | N_LANES(dsi->lanes)); 
>>  } 
>> 
>>  static void dw_mipi_dsi_dphy_init(struct dw_mipi_dsi *dsi) { 
>>         /* Clear PHY state */ 
>> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_DISFORCEPLL | 
>> PHY_DISABLECLK -                 | PHY_RSTZ | PHY_SHUTDOWNZ); - 
>> dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR); - 
>> dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLR); - 
>> dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR); + 
>> regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_DISFORCEPLL | 
>> PHY_DISABLECLK +                    | PHY_RSTZ | 
>> PHY_SHUTDOWNZ); +       regmap_write(dsi->regs, 
>> DSI_PHY_TST_CTRL0, PHY_UNTESTCLR); + 
>> regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_TESTCLR); + 
>> regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR); 
>>  } 
>> 
>>  static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi *dsi) { 
>> -       u32 val; +       u32 val = 0; 
> 
> ditto 
> 
>>         int ret; 
>> 
>> -       dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | 
>> PHY_ENABLECLK | -                 PHY_UNRSTZ | 
>> PHY_UNSHUTDOWNZ); +       regmap_write(dsi->regs, DSI_PHY_RSTZ, 
>> PHY_ENFORCEPLL | PHY_ENABLECLK | + 
>> PHY_UNRSTZ | PHY_UNSHUTDOWNZ); 
>> 
>> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, 
>> val, -                                val & PHY_LOCK, 1000, 
>> PHY_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS, + 
>> val, val & PHY_LOCK, + 
>> 1000, PHY_STATUS_TIMEOUT_US); 
>>         if (ret) 
>>                 DRM_DEBUG_DRIVER("failed to wait phy lock 
>>                 state\n"); 
>> 
>> -       ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, - 
>> val, val & PHY_STOP_STATE_CLK_LANE, 1000, - 
>> PHY_STATUS_TIMEOUT_US); +       ret = 
>> regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS, + 
>> val, val & PHY_STOP_STATE_CLK_LANE, 1000, + 
>> PHY_STATUS_TIMEOUT_US); 
>>         if (ret) 
>>                 DRM_DEBUG_DRIVER("failed to wait phy clk lane 
>>                 stop state\n"); 
>>  } 
>> 
>>  static void dw_mipi_dsi_clear_err(struct dw_mipi_dsi *dsi) { 
>> -       dsi_read(dsi, DSI_INT_ST0); -       dsi_read(dsi, 
>> DSI_INT_ST1); -       dsi_write(dsi, DSI_INT_MSK0, 0); - 
>> dsi_write(dsi, DSI_INT_MSK1, 0); +       u32 val; + + 
>> regmap_read(dsi->regs, DSI_INT_ST0, &val); + 
>> regmap_read(dsi->regs, DSI_INT_ST1, &val); + 
>> regmap_write(dsi->regs, DSI_INT_MSK0, 0); + 
>> regmap_write(dsi->regs, DSI_INT_MSK1, 0); 
>>  } 
>> 
>>  static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge 
>>  *bridge) 
>> @@ -989,6 +997,14 @@ static void 
>> dw_mipi_dsi_debugfs_remove(struct dw_mipi_dsi *dsi) { } 
>> 
>>  #endif /* CONFIG_DEBUG_FS */ 
>> 
>> +static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi 
>> *dsi) +{ +       regmap_read(dsi->regs, DSI_VERSION, 
>> &dsi->hw_version); +       dsi->hw_version &= VERSION; + 
>> if (!dsi->hw_version) +               dev_err(dsi->dev, "Failed 
>> to read DSI hw version register\n"); 
> 
> Is this an error that should be ignored? If you can't get the HW 
> version, probably, there is something wrong with your hardware 
> so, don't you need to return an error? 
> 

After thinking a bit more about it, that error should be a 
warning.

I added it because in some cases (for eg. if the peripheral clock 
is disabled) the reads can return 0 which is obviously an invalid 
version and the bridge will error in the next step when not 
finding a layout.

So I'll make this a warning in v7 and explicitely mention that 
reads version == 0 can be caused by a disabled pclk.

>> +}
>> +
>>  static struct dw_mipi_dsi *
>>  __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                     const struct dw_mipi_dsi_plat_data *plat_data)
>> @@ -1020,6 +1036,14 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                 dsi->base = plat_data->base;
>>         }
>>
>> +       dsi->regs = devm_regmap_init_mmio(dev, dsi->base,
>> +                                         &dw_mipi_dsi_regmap_cfg);
>> +       if (IS_ERR(dsi->regs)) {
>> +               ret = PTR_ERR(dsi->regs);
>> +               DRM_ERROR("Failed to create DW MIPI DSI regmap: %d\n", ret);
>> +               return ERR_PTR(ret);
>> +       }
>> +
>>         dsi->pclk = devm_clk_get(dev, "pclk");
>>         if (IS_ERR(dsi->pclk)) {
>>                 ret = PTR_ERR(dsi->pclk);
>> @@ -1055,6 +1079,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
>>                 clk_disable_unprepare(dsi->pclk);
>>         }
>>
>> +       dw_mipi_dsi_get_hw_version(dsi);
>> +
>>         dw_mipi_dsi_debugfs_init(dsi);
>>         pm_runtime_enable(dev);
>>
>> --
>> 2.26.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Enric Balletbo Serra April 16, 2020, 8:15 p.m. UTC | #4
Hi Adrian,

[snip]

> >>
> >> +static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi
> >> *dsi) +{ +       regmap_read(dsi->regs, DSI_VERSION,
> >> &dsi->hw_version); +       dsi->hw_version &= VERSION; +
> >> if (!dsi->hw_version) +               dev_err(dsi->dev, "Failed
> >> to read DSI hw version register\n");
> >
> > Is this an error that should be ignored? If you can't get the HW
> > version, probably, there is something wrong with your hardware
> > so, don't you need to return an error?
> >
>
> After thinking a bit more about it, that error should be a
> warning.
>
> I added it because in some cases (for eg. if the peripheral clock
> is disabled) the reads can return 0 which is obviously an invalid
> version and the bridge will error in the next step when not
> finding a layout.
>

If you'll error anyway, why wait? IIUC at this point the clock *must*
be enabled, and if not, something is wrong with the driver, I don't
see any advantage on delay the error. do you have a use case where
this is called and peripheral clock disabled?

> So I'll make this a warning in v7 and explicitely mention that
> reads version == 0 can be caused by a disabled pclk.
>

-- Enric
Adrian Ratiu April 17, 2020, 8:07 a.m. UTC | #5
On Thu, 16 Apr 2020, Enric Balletbo Serra <eballetbo@gmail.com> 
wrote:
> Hi Adrian, 
> 
> [snip] 
> 
>> >> 
>> >> +static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi 
>> >> *dsi) +{ +       regmap_read(dsi->regs, DSI_VERSION, 
>> >> &dsi->hw_version); +       dsi->hw_version &= VERSION; + if 
>> >> (!dsi->hw_version) +               dev_err(dsi->dev, "Failed 
>> >> to read DSI hw version register\n"); 
>> > 
>> > Is this an error that should be ignored? If you can't get the 
>> > HW version, probably, there is something wrong with your 
>> > hardware so, don't you need to return an error? 
>> > 
>> 
>> After thinking a bit more about it, that error should be a 
>> warning. 
>> 
>> I added it because in some cases (for eg. if the peripheral 
>> clock is disabled) the reads can return 0 which is obviously an 
>> invalid version and the bridge will error in the next step when 
>> not finding a layout. 
>> 
> 
> If you'll error anyway, why wait? IIUC at this point the clock 
> *must* be enabled, and if not, something is wrong with the 
> driver, I don't see any advantage on delay the error. do you 
> have a use case where this is called and peripheral clock 
> disabled? 

There should be no real use-case (maybe malfunctioning HW), and we 
could error out here to catch driver bugs ASAP, so I'll go this 
route then :)

Thank you, much appreciated!

>
>> So I'll make this a warning in v7 and explicitely mention that
>> reads version == 0 can be caused by a disabled pclk.
>>
>
> -- Enric
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 5ef0f154aa7b..6d9e2f21c9cc 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -15,6 +15,7 @@ 
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 
 #include <video/mipi_display.h>
@@ -227,6 +228,7 @@  struct dw_mipi_dsi {
 	struct drm_bridge *panel_bridge;
 	struct device *dev;
 	void __iomem *base;
+	struct regmap *regs;
 
 	struct clk *pclk;
 
@@ -235,6 +237,7 @@  struct dw_mipi_dsi {
 	u32 lanes;
 	u32 format;
 	unsigned long mode_flags;
+	u32 hw_version;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
@@ -249,6 +252,13 @@  struct dw_mipi_dsi {
 	const struct dw_mipi_dsi_plat_data *plat_data;
 };
 
+static const struct regmap_config dw_mipi_dsi_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.name = "dw-mipi-dsi",
+};
+
 /*
  * Check if either a link to a master or slave is present
  */
@@ -280,16 +290,6 @@  static inline struct dw_mipi_dsi *bridge_to_dsi(struct drm_bridge *bridge)
 	return container_of(bridge, struct dw_mipi_dsi, bridge);
 }
 
-static inline void dsi_write(struct dw_mipi_dsi *dsi, u32 reg, u32 val)
-{
-	writel(val, dsi->base + reg);
-}
-
-static inline u32 dsi_read(struct dw_mipi_dsi *dsi, u32 reg)
-{
-	return readl(dsi->base + reg);
-}
-
 static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
 				   struct mipi_dsi_device *device)
 {
@@ -366,29 +366,29 @@  static void dw_mipi_message_config(struct dw_mipi_dsi *dsi,
 	if (lpm)
 		val |= CMD_MODE_ALL_LP;
 
-	dsi_write(dsi, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
-	dsi_write(dsi, DSI_CMD_MODE_CFG, val);
+	regmap_write(dsi->regs, DSI_LPCLK_CTRL, lpm ? 0 : PHY_TXREQUESTCLKHS);
+	regmap_write(dsi->regs, DSI_CMD_MODE_CFG, val);
 }
 
 static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
 {
 	int ret;
-	u32 val, mask;
+	u32 val = 0, mask;
 
-	ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
-				 val, !(val & GEN_CMD_FULL), 1000,
-				 CMD_PKT_STATUS_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
+				       val, !(val & GEN_CMD_FULL), 1000,
+				       CMD_PKT_STATUS_TIMEOUT_US);
 	if (ret) {
 		dev_err(dsi->dev, "failed to get available command FIFO\n");
 		return ret;
 	}
 
-	dsi_write(dsi, DSI_GEN_HDR, hdr_val);
+	regmap_write(dsi->regs, DSI_GEN_HDR, hdr_val);
 
 	mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY;
-	ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
-				 val, (val & mask) == mask,
-				 1000, CMD_PKT_STATUS_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
+				       val, (val & mask) == mask,
+				       1000, CMD_PKT_STATUS_TIMEOUT_US);
 	if (ret) {
 		dev_err(dsi->dev, "failed to write command FIFO\n");
 		return ret;
@@ -403,24 +403,26 @@  static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi,
 	const u8 *tx_buf = packet->payload;
 	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
 	__le32 word;
-	u32 val;
+	u32 val = 0;
 
 	while (len) {
 		if (len < pld_data_bytes) {
 			word = 0;
 			memcpy(&word, tx_buf, len);
-			dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word));
+			regmap_write(dsi->regs, DSI_GEN_PLD_DATA,
+				     le32_to_cpu(word));
 			len = 0;
 		} else {
 			memcpy(&word, tx_buf, pld_data_bytes);
-			dsi_write(dsi, DSI_GEN_PLD_DATA, le32_to_cpu(word));
+			regmap_write(dsi->regs, DSI_GEN_PLD_DATA,
+				     le32_to_cpu(word));
 			tx_buf += pld_data_bytes;
 			len -= pld_data_bytes;
 		}
 
-		ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
-					 val, !(val & GEN_PLD_W_FULL), 1000,
-					 CMD_PKT_STATUS_TIMEOUT_US);
+		ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
+					       val, !(val & GEN_PLD_W_FULL),
+					       1000, CMD_PKT_STATUS_TIMEOUT_US);
 		if (ret) {
 			dev_err(dsi->dev,
 				"failed to get available write payload FIFO\n");
@@ -438,12 +440,12 @@  static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
 {
 	int i, j, ret, len = msg->rx_len;
 	u8 *buf = msg->rx_buf;
-	u32 val;
+	u32 val = 0;
 
 	/* Wait end of the read operation */
-	ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
-				 val, !(val & GEN_RD_CMD_BUSY),
-				 1000, CMD_PKT_STATUS_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
+				       val, !(val & GEN_RD_CMD_BUSY),
+				       1000, CMD_PKT_STATUS_TIMEOUT_US);
 	if (ret) {
 		dev_err(dsi->dev, "Timeout during read operation\n");
 		return ret;
@@ -451,15 +453,15 @@  static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
 
 	for (i = 0; i < len; i += 4) {
 		/* Read fifo must not be empty before all bytes are read */
-		ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
-					 val, !(val & GEN_PLD_R_EMPTY),
-					 1000, CMD_PKT_STATUS_TIMEOUT_US);
+		ret = regmap_read_poll_timeout(dsi->regs, DSI_CMD_PKT_STATUS,
+					       val, !(val & GEN_PLD_R_EMPTY),
+					       1000, CMD_PKT_STATUS_TIMEOUT_US);
 		if (ret) {
 			dev_err(dsi->dev, "Read payload FIFO is empty\n");
 			return ret;
 		}
 
-		val = dsi_read(dsi, DSI_GEN_PLD_DATA);
+		regmap_read(dsi->regs, DSI_GEN_PLD_DATA, &val);
 		for (j = 0; j < 4 && j + i < len; j++)
 			buf[i + j] = val >> (8 * j);
 	}
@@ -536,29 +538,29 @@  static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
 	}
 #endif /* CONFIG_DEBUG_FS */
 
-	dsi_write(dsi, DSI_VID_MODE_CFG, val);
+	regmap_write(dsi->regs, DSI_VID_MODE_CFG, val);
 }
 
 static void dw_mipi_dsi_set_mode(struct dw_mipi_dsi *dsi,
 				 unsigned long mode_flags)
 {
-	dsi_write(dsi, DSI_PWR_UP, RESET);
+	regmap_write(dsi->regs, DSI_PWR_UP, RESET);
 
 	if (mode_flags & MIPI_DSI_MODE_VIDEO) {
-		dsi_write(dsi, DSI_MODE_CFG, ENABLE_VIDEO_MODE);
+		regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_VIDEO_MODE);
 		dw_mipi_dsi_video_mode_config(dsi);
-		dsi_write(dsi, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
+		regmap_write(dsi->regs, DSI_LPCLK_CTRL, PHY_TXREQUESTCLKHS);
 	} else {
-		dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
+		regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE);
 	}
 
-	dsi_write(dsi, DSI_PWR_UP, POWERUP);
+	regmap_write(dsi->regs, DSI_PWR_UP, POWERUP);
 }
 
 static void dw_mipi_dsi_disable(struct dw_mipi_dsi *dsi)
 {
-	dsi_write(dsi, DSI_PWR_UP, RESET);
-	dsi_write(dsi, DSI_PHY_RSTZ, PHY_RSTZ);
+	regmap_write(dsi->regs, DSI_PWR_UP, RESET);
+	regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_RSTZ);
 }
 
 static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
@@ -573,14 +575,14 @@  static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
 	 */
 	u32 esc_clk_division = (dsi->lane_mbps >> 3) / 20 + 1;
 
-	dsi_write(dsi, DSI_PWR_UP, RESET);
+	regmap_write(dsi->regs, DSI_PWR_UP, RESET);
 
 	/*
 	 * TODO dw drv improvements
 	 * timeout clock division should be computed with the
 	 * high speed transmission counter timeout and byte lane...
 	 */
-	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
+	regmap_write(dsi->regs, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
 		  TX_ESC_CLK_DIVISION(esc_clk_division));
 }
 
@@ -609,22 +611,22 @@  static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi,
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		val |= HSYNC_ACTIVE_LOW;
 
-	dsi_write(dsi, DSI_DPI_VCID, DPI_VCID(dsi->channel));
-	dsi_write(dsi, DSI_DPI_COLOR_CODING, color);
-	dsi_write(dsi, DSI_DPI_CFG_POL, val);
+	regmap_write(dsi->regs, DSI_DPI_VCID, DPI_VCID(dsi->channel));
+	regmap_write(dsi->regs, DSI_DPI_COLOR_CODING, color);
+	regmap_write(dsi->regs, DSI_DPI_CFG_POL, val);
 	/*
 	 * TODO dw drv improvements
 	 * largest packet sizes during hfp or during vsa/vpb/vfp
 	 * should be computed according to byte lane, lane number and only
 	 * if sending lp cmds in high speed is enable (PHY_TXREQUESTCLKHS)
 	 */
-	dsi_write(dsi, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4)
+	regmap_write(dsi->regs, DSI_DPI_LP_CMD_TIM, OUTVACT_LPCMD_TIME(4)
 		  | INVACT_LPCMD_TIME(4));
 }
 
 static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi)
 {
-	dsi_write(dsi, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | BTA_EN);
+	regmap_write(dsi->regs, DSI_PCKHDL_CFG, CRC_RX_EN | ECC_RX_EN | BTA_EN);
 }
 
 static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
@@ -638,7 +640,7 @@  static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi,
 	 * non-burst video modes, see dw_mipi_dsi_video_mode_config()...
 	 */
 
-	dsi_write(dsi, DSI_VID_PKT_SIZE,
+	regmap_write(dsi->regs, DSI_VID_PKT_SIZE,
 		       dw_mipi_is_dual_mode(dsi) ?
 				VID_PKT_SIZE(mode->hdisplay / 2) :
 				VID_PKT_SIZE(mode->hdisplay));
@@ -651,14 +653,15 @@  static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
 	 * compute high speed transmission counter timeout according
 	 * to the timeout clock division (TO_CLK_DIVISION) and byte lane...
 	 */
-	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
+	regmap_write(dsi->regs, DSI_TO_CNT_CFG,
+		     HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
 	/*
 	 * TODO dw drv improvements
 	 * the Bus-Turn-Around Timeout Counter should be computed
 	 * according to byte lane...
 	 */
-	dsi_write(dsi, DSI_BTA_TO_CNT, 0xd00);
-	dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
+	regmap_write(dsi->regs, DSI_BTA_TO_CNT, 0xd00);
+	regmap_write(dsi->regs, DSI_MODE_CFG, ENABLE_CMD_MODE);
 }
 
 /* Get lane byte clock cycles. */
@@ -692,13 +695,13 @@  static void dw_mipi_dsi_line_timer_config(struct dw_mipi_dsi *dsi,
 	 * computations below may be improved...
 	 */
 	lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, htotal);
-	dsi_write(dsi, DSI_VID_HLINE_TIME, lbcc);
+	regmap_write(dsi->regs, DSI_VID_HLINE_TIME, lbcc);
 
 	lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hsa);
-	dsi_write(dsi, DSI_VID_HSA_TIME, lbcc);
+	regmap_write(dsi->regs, DSI_VID_HSA_TIME, lbcc);
 
 	lbcc = dw_mipi_dsi_get_hcomponent_lbcc(dsi, mode, hbp);
-	dsi_write(dsi, DSI_VID_HBP_TIME, lbcc);
+	regmap_write(dsi->regs, DSI_VID_HBP_TIME, lbcc);
 }
 
 static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi,
@@ -711,10 +714,10 @@  static void dw_mipi_dsi_vertical_timing_config(struct dw_mipi_dsi *dsi,
 	vfp = mode->vsync_start - mode->vdisplay;
 	vbp = mode->vtotal - mode->vsync_end;
 
-	dsi_write(dsi, DSI_VID_VACTIVE_LINES, vactive);
-	dsi_write(dsi, DSI_VID_VSA_LINES, vsa);
-	dsi_write(dsi, DSI_VID_VFP_LINES, vfp);
-	dsi_write(dsi, DSI_VID_VBP_LINES, vbp);
+	regmap_write(dsi->regs, DSI_VID_VACTIVE_LINES, vactive);
+	regmap_write(dsi->regs, DSI_VID_VSA_LINES, vsa);
+	regmap_write(dsi->regs, DSI_VID_VFP_LINES, vfp);
+	regmap_write(dsi->regs, DSI_VID_VBP_LINES, vbp);
 }
 
 static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi)
@@ -737,23 +740,25 @@  static void dw_mipi_dsi_dphy_timing_config(struct dw_mipi_dsi *dsi)
 	 * DSI_CMD_MODE_CFG.MAX_RD_PKT_SIZE_LP (see CMD_MODE_ALL_LP)
 	 */
 
-	hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
+	regmap_read(dsi->regs, DSI_VERSION, &hw_version);
+	hw_version &= VERSION;
 
 	if (hw_version >= HWVER_131) {
-		dsi_write(dsi, DSI_PHY_TMR_CFG,
-			  PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
-			  PHY_LP2HS_TIME_V131(timing.data_lp2hs));
-		dsi_write(dsi, DSI_PHY_TMR_RD_CFG, MAX_RD_TIME_V131(10000));
+		regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
+			     PHY_HS2LP_TIME_V131(timing.data_hs2lp) |
+			     PHY_LP2HS_TIME_V131(timing.data_lp2hs));
+		regmap_write(dsi->regs, DSI_PHY_TMR_RD_CFG,
+			     MAX_RD_TIME_V131(10000));
 	} else {
-		dsi_write(dsi, DSI_PHY_TMR_CFG,
-			  PHY_HS2LP_TIME(timing.data_hs2lp) |
-			  PHY_LP2HS_TIME(timing.data_lp2hs) |
-			  MAX_RD_TIME(10000));
+		regmap_write(dsi->regs, DSI_PHY_TMR_CFG,
+			     PHY_HS2LP_TIME(timing.data_hs2lp) |
+			     PHY_LP2HS_TIME(timing.data_lp2hs) |
+			     MAX_RD_TIME(10000));
 	}
 
-	dsi_write(dsi, DSI_PHY_TMR_LPCLK_CFG,
-		  PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
-		  PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
+	regmap_write(dsi->regs, DSI_PHY_TMR_LPCLK_CFG,
+		     PHY_CLKHS2LP_TIME(timing.clk_hs2lp) |
+		     PHY_CLKLP2HS_TIME(timing.clk_lp2hs));
 }
 
 static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
@@ -763,46 +768,49 @@  static void dw_mipi_dsi_dphy_interface_config(struct dw_mipi_dsi *dsi)
 	 * stop wait time should be the maximum between host dsi
 	 * and panel stop wait times
 	 */
-	dsi_write(dsi, DSI_PHY_IF_CFG, PHY_STOP_WAIT_TIME(0x20) |
-		  N_LANES(dsi->lanes));
+	regmap_write(dsi->regs, DSI_PHY_IF_CFG,
+		     PHY_STOP_WAIT_TIME(0x20) | N_LANES(dsi->lanes));
 }
 
 static void dw_mipi_dsi_dphy_init(struct dw_mipi_dsi *dsi)
 {
 	/* Clear PHY state */
-	dsi_write(dsi, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
-		  | PHY_RSTZ | PHY_SHUTDOWNZ);
-	dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
-	dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
-	dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
+	regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_DISFORCEPLL | PHY_DISABLECLK
+		     | PHY_RSTZ | PHY_SHUTDOWNZ);
+	regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
+	regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_TESTCLR);
+	regmap_write(dsi->regs, DSI_PHY_TST_CTRL0, PHY_UNTESTCLR);
 }
 
 static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi *dsi)
 {
-	u32 val;
+	u32 val = 0;
 	int ret;
 
-	dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
-		  PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
+	regmap_write(dsi->regs, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
+		     PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
 
-	ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS, val,
-				 val & PHY_LOCK, 1000, PHY_STATUS_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
+				       val, val & PHY_LOCK,
+				       1000, PHY_STATUS_TIMEOUT_US);
 	if (ret)
 		DRM_DEBUG_DRIVER("failed to wait phy lock state\n");
 
-	ret = readl_poll_timeout(dsi->base + DSI_PHY_STATUS,
-				 val, val & PHY_STOP_STATE_CLK_LANE, 1000,
-				 PHY_STATUS_TIMEOUT_US);
+	ret = regmap_read_poll_timeout(dsi->regs, DSI_PHY_STATUS,
+				       val, val & PHY_STOP_STATE_CLK_LANE, 1000,
+				       PHY_STATUS_TIMEOUT_US);
 	if (ret)
 		DRM_DEBUG_DRIVER("failed to wait phy clk lane stop state\n");
 }
 
 static void dw_mipi_dsi_clear_err(struct dw_mipi_dsi *dsi)
 {
-	dsi_read(dsi, DSI_INT_ST0);
-	dsi_read(dsi, DSI_INT_ST1);
-	dsi_write(dsi, DSI_INT_MSK0, 0);
-	dsi_write(dsi, DSI_INT_MSK1, 0);
+	u32 val;
+
+	regmap_read(dsi->regs, DSI_INT_ST0, &val);
+	regmap_read(dsi->regs, DSI_INT_ST1, &val);
+	regmap_write(dsi->regs, DSI_INT_MSK0, 0);
+	regmap_write(dsi->regs, DSI_INT_MSK1, 0);
 }
 
 static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge)
@@ -989,6 +997,14 @@  static void dw_mipi_dsi_debugfs_remove(struct dw_mipi_dsi *dsi) { }
 
 #endif /* CONFIG_DEBUG_FS */
 
+static void dw_mipi_dsi_get_hw_version(struct dw_mipi_dsi *dsi)
+{
+	regmap_read(dsi->regs, DSI_VERSION, &dsi->hw_version);
+	dsi->hw_version &= VERSION;
+	if (!dsi->hw_version)
+		dev_err(dsi->dev, "Failed to read DSI hw version register\n");
+}
+
 static struct dw_mipi_dsi *
 __dw_mipi_dsi_probe(struct platform_device *pdev,
 		    const struct dw_mipi_dsi_plat_data *plat_data)
@@ -1020,6 +1036,14 @@  __dw_mipi_dsi_probe(struct platform_device *pdev,
 		dsi->base = plat_data->base;
 	}
 
+	dsi->regs = devm_regmap_init_mmio(dev, dsi->base,
+					  &dw_mipi_dsi_regmap_cfg);
+	if (IS_ERR(dsi->regs)) {
+		ret = PTR_ERR(dsi->regs);
+		DRM_ERROR("Failed to create DW MIPI DSI regmap: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
 	dsi->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(dsi->pclk)) {
 		ret = PTR_ERR(dsi->pclk);
@@ -1055,6 +1079,8 @@  __dw_mipi_dsi_probe(struct platform_device *pdev,
 		clk_disable_unprepare(dsi->pclk);
 	}
 
+	dw_mipi_dsi_get_hw_version(dsi);
+
 	dw_mipi_dsi_debugfs_init(dsi);
 	pm_runtime_enable(dev);