Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support

Hi Guido,

Thanks for your work on this driver!

On Wed, Jul 24, 2019 at 12:52 PM Guido Günther agx@sigxcpu.org wrote:

--- /dev/null +++ b/drivers/gpu/drm/bridge/imx-nwl/Kconfig @@ -0,0 +1,15 @@ +config DRM_IMX_NWL_DSI

•   tristate "Support for Northwest Logic MIPI DSI Host controller"
  

•   depends on DRM && (ARCH_MXC || ARCH_MULTIPLATFORM || COMPILE_TEST)
  

This IP could potentially be found on other SoCs, so no need to make it depend on ARCH_MXC.

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_print.h> +#include <drm/drm_probe_helper.h> +#include <linux/clk-provider.h> +#include <linux/clk.h> +#include <linux/component.h> +#include <linux/gpio/consumer.h>

I did not find gpio AP used in this driver.

+static void imx_nwl_dsi_set_clocks(struct imx_nwl_dsi *dsi, bool enable)

Better make it to return 'int' instead...

+{

•   struct device *dev = dsi->dev;
  

•   const char *id;
  

•   struct clk *clk;
  

•   unsigned long new_rate, cur_rate;
  

•   bool enabled;
  

•   size_t i;
  

•   int ret;
  

•   DRM_DEV_DEBUG_DRIVER(dev, "%sabling platform clocks",
  

Please remove the letter 's' from 'sabling'.

•                        enable ? "en" : "dis");
  

•                   ret = clk_prepare_enable(clk);
  

•                   if (ret < 0) {
  

•                           DRM_DEV_ERROR(dev, "Failed to enable clock %s",
  

•                                         id);
  

and propagate the error in case of clk_prepare_enable() failure.

•                   }
  

•                   dsi->clk_config[i].enabled = true;
  

•                   cur_rate = clk_get_rate(clk);
  

•                   DRM_DEV_DEBUG_DRIVER(
  

•                           dev, "Enabled %s clk (rate: req=%lu act=%lu)\n",
  

•                           id, new_rate, cur_rate);
  

•           } else if (enabled) {
  

•                   clk_disable_unprepare(clk);
  

•                   dsi->clk_config[i].enabled = false;
  

•                   DRM_DEV_DEBUG_DRIVER(dev, "Disabled %s clk\n", id);
  

•           }
  

•   }
  

+}

+static void imx_nwl_dsi_enable(struct imx_nwl_dsi *dsi)

Same here. Please return 'int' instead.

+{

•   struct device *dev = dsi->dev;
  

•   int ret;
  

•   imx_nwl_dsi_set_clocks(dsi, true);
  

•   ret = dsi->pdata->poweron(dsi);
  

•   if (ret < 0)
  

•           DRM_DEV_ERROR(dev, "Failed to power on DSI (%d)\n", ret);
  

If the power domain failed to turn on, it is better to propagate the error.

•   phy_ref_rate = clk_get_rate(dsi->phy_ref_clk);
  

•   DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate);
  

•   if (ret < 0) {
  

This check looks wrong. At this point ret is always 0.

•           DRM_DEV_ERROR(dsi->dev,
  

•                         "Cannot setup PHY for mode: %ux%u @%d Hz\n",
  

•                         adjusted_mode->hdisplay, adjusted_mode->vdisplay,
  

•                         adjusted_mode->clock);
  

•           DRM_DEV_ERROR(dsi->dev, "PHY ref clk: %lu, bit clk: %lu\n",
  

•                         phy_ref_rate, new_cfg.mipi_dphy.hs_clk_rate);
  

•   } else {
  

•           /* Save the new desired phy config */
  

•           memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
  

•   }
  

•   /* LCDIF + NWL needs active high sync */
  

Would this still work if DCSS is used instead?

•   adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
  

•   adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
  

•   drm_display_mode_to_videomode(adjusted_mode, &dsi->vm);
  

•   drm_mode_debug_printmodeline(adjusted_mode);
  

•   return ret == 0;
  

At this point ret is always 0.

+static void imx_nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge) +{

•   struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
  

•   if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
  

•           return;
  

•   imx_nwl_select_input_source(dsi);
  

This function is i.MX8M specific, so better protect it to run only for the i.MX8M variant.

•   pm_runtime_get_sync(dsi->dev);
  

•   imx_nwl_dsi_enable(dsi);
  

•   nwl_dsi_enable(dsi);
  

Please check the error and propagate in the case of failure.

•   dsi->dpms_mode = DRM_MODE_DPMS_ON;
  

+}

•   dsi->csr = syscon_regmap_lookup_by_phandle(np, "csr");
  

•   if (IS_ERR(dsi->csr) && dsi->pdata->ext_regs & IMX_REG_CSR) {
  

•           ret = PTR_ERR(dsi->csr);
  

•           DRM_DEV_ERROR(dsi->dev, "Failed to get CSR regmap: %d\n",
  

In this function (and globally in the driver) there is a mix of DRM_DEV_ERROR() and dev_err().

Can we just pick one of the two and use it consistently?

Not sure what is the norm in drm code, but IMHO dev_err() looks prettier :-)

•   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  

•   base = devm_ioremap_resource(dsi->dev, res);
  

Could use devm_platform_ioremap_resource(), which makes it simpler.

+err_cleanup:

•   devm_free_irq(dev, dsi->irq, dsi);
  

No need to call devm_free_irq() here. The devm functions do not need to be freed on probe.

diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c new file mode 100644 index 000000000000..0e1463af162f --- /dev/null +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c @@ -0,0 +1,745 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • NWL DSI host driver
• • Copyright (C) 2017 NXP
• • Copyright (C) 2019 Purism SPC
• */

+#include <asm/unaligned.h>

Is this asm header required?

+/*

• • DSI Video mode
• */

Single line comment would suffice.

+#define VIDEO_MODE_BURST_MODE_WITH_SYNC_PULSES 0 +#define VIDEO_MODE_NON_BURST_MODE_WITH_SYNC_EVENTS BIT(0) +#define VIDEO_MODE_BURST_MODE BIT(1)

+/*

• • DPI color coding
• */

Ditto.

+#define DPI_16_BIT_565_PACKED 0 +#define DPI_16_BIT_565_ALIGNED 1 +#define DPI_16_BIT_565_SHIFTED 2 +#define DPI_18_BIT_PACKED 3 +#define DPI_18_BIT_ALIGNED 4 +#define DPI_24_BIT 5

+/*

• • DPI Pixel format
• */

Ditto.

+#define PIXEL_FORMAT_16 0 +#define PIXEL_FORMAT_18 BIT(0) +#define PIXEL_FORMAT_18L BIT(1) +#define PIXEL_FORMAT_24 (BIT(0) | BIT(1))

+enum transfer_direction { DSI_PACKET_SEND, DSI_PACKET_RECEIVE };

+struct mipi_dsi_transfer {

•   const struct mipi_dsi_msg *msg;
  

•   struct mipi_dsi_packet packet;
  

•   struct completion completed;
  

•   int status; /* status of transmission */
  

•   enum transfer_direction direction;
  

•   bool need_bta;
  

•   u8 cmd;
  

•   u16 rx_word_count;
  

•   size_t tx_len; /* bytes sent */
  

•   size_t rx_len; /* bytes received */
  

+};

The comments here are kind of obvious, so I would just remove them.

+static inline int nwl_dsi_write(struct imx_nwl_dsi *dsi, unsigned int reg,

inline can be dropped.

•                           u32 val)
  

+{

•   int ret;
  

•   ret = regmap_write(dsi->regmap, reg, val);
  

•   if (ret < 0)
  

•           DRM_DEV_ERROR(dsi->dev,
  

•                         "Failed to write NWL DSI reg 0x%x: %d\n", reg,
  

•                         ret);
  

•   return ret;
  

+}

+static inline u32 nwl_dsi_read(struct imx_nwl_dsi *dsi, u32 reg)

Same here.

+{

•   unsigned int val;
  

•   int ret;
  

•   ret = regmap_read(dsi->regmap, reg, &val);
  

•   if (ret < 0)
  

•           DRM_DEV_ERROR(dsi->dev, "Failed to read NWL DSI reg 0x%x: %d\n",
  

•                         reg, ret);
  

•   return val;
  

+}

It seems that we could simply use regmap_read/write() directly instead of these functions.

+int nwl_dsi_get_dphy_params(struct imx_nwl_dsi *dsi,

•                       const struct drm_display_mode *mode,
  

•                       union phy_configure_opts *phy_opts)
  

+{

•   unsigned long rate;
  

•   if (dsi->lanes < 1 || dsi->lanes > 4)
  

•           return -EINVAL;
  

•   /*
  

•    * So far the DPHY spec minimal timings work for both mixel
  

•    * dphy and nwl dsi host
  

•    */
  

•   phy_mipi_dphy_get_default_config(
  

•           mode->crtc_clock * 1000,
  

•           mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes,
  

•           &phy_opts->mipi_dphy);
  

•   rate = clk_get_rate(dsi->tx_esc_clk);
  

•   DRM_DEV_DEBUG_DRIVER(dsi->dev, "LP clk is @%lu Hz\n", rate);
  

•   phy_opts->mipi_dphy.lp_clk_rate = rate;
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(nwl_dsi_get_dphy_params);

Does it really need to be exported? Why can't it be placed inside nwl-drv.c and be made static?

+/**

/* is enough

• • ui2bc - UI time periods to byte clock cycles
• */

+static u32 ui2bc(struct imx_nwl_dsi *dsi, unsigned long long ui) +{

•   int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
  

•   return DIV_ROUND_UP(ui * dsi->lanes, dsi->vm.pixelclock * bpp);
  

+}

+#define USEC_PER_SEC 1000000L

This definition already exists in include/linux/time64.h. No need to redefine it.

+static int nwl_dsi_enable_tx_clock(struct imx_nwl_dsi *dsi) +{

•   struct device *dev = dsi->dev;
  

•   int ret;
  

•   ret = clk_prepare_enable(dsi->tx_esc_clk);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to enable tx_esc clk: %d\n", ret);
  

•           return ret;
  

•   }
  

•   DRM_DEV_DEBUG_DRIVER(dev, "Enabled tx_esc clk @%lu Hz\n",
  

•                        clk_get_rate(dsi->tx_esc_clk));
  

•   return 0;
  

+}

Do we really need this function? It looks like it would be simpler just to call clk_prepare_enable() directly.

+static int nwl_dsi_enable_rx_clock(struct imx_nwl_dsi *dsi) +{

•   struct device *dev = dsi->dev;
  

•   int ret;
  

•   ret = clk_prepare_enable(dsi->rx_esc_clk);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to enable rx_esc clk: %d\n", ret);
  

•           return ret;
  

•   }
  

•   DRM_DEV_DEBUG_DRIVER(dev, "Enabled rx_esc clk @%lu Hz\n",
  

•                        clk_get_rate(dsi->rx_esc_clk));
  

•   return 0;
  

+}

Same here.

+static ssize_t nwl_dsi_host_transfer(struct mipi_dsi_host *dsi_host,

•                                const struct mipi_dsi_msg *msg)
  

+{

•   struct imx_nwl_dsi *dsi =
  

•           container_of(dsi_host, struct imx_nwl_dsi, dsi_host);
  

•   struct mipi_dsi_transfer xfer;
  

•   ssize_t ret = 0;
  

•   /* Create packet to be sent */
  

•   dsi->xfer = &xfer;
  

•   ret = mipi_dsi_create_packet(&xfer.packet, msg);
  

•   if (ret < 0) {
  

•           dsi->xfer = NULL;
  

•           return ret;
  

•   }
  

•   if ((msg->type & MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM ||
  

•        msg->type & MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM ||
  

•        msg->type & MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM ||
  

•        msg->type & MIPI_DSI_DCS_READ) &&
  

•       msg->rx_len > 0 && msg->rx_buf != NULL)
  

•           xfer.direction = DSI_PACKET_RECEIVE;
  

•   else
  

•           xfer.direction = DSI_PACKET_SEND;
  

•   xfer.need_bta = (xfer.direction == DSI_PACKET_RECEIVE);
  

•   xfer.need_bta |= (msg->flags & MIPI_DSI_MSG_REQ_ACK) ? 1 : 0;
  

•   xfer.msg = msg;
  

•   xfer.status = -ETIMEDOUT;
  

•   xfer.rx_word_count = 0;
  

•   xfer.rx_len = 0;
  

•   xfer.cmd = 0x00;
  

•   if (msg->tx_len > 0)
  

•           xfer.cmd = ((u8 *)(msg->tx_buf))[0];
  

•   init_completion(&xfer.completed);
  

•   nwl_dsi_enable_rx_clock(dsi);
  

This may fail, so better check the error.

ret = clk_prepare_enable() if (ret < 0) return ret;

+irqreturn_t nwl_dsi_irq_handler(int irq, void *data) +{

•   u32 irq_status;
  

•   struct imx_nwl_dsi *dsi = data;
  

•   irq_status = nwl_dsi_read(dsi, IRQ_STATUS);
  

•   if (irq_status & TX_PKT_DONE || irq_status & RX_PKT_HDR_RCVD ||
  

•       irq_status & RX_PKT_PAYLOAD_DATA_RCVD)
  

•           nwl_dsi_finish_transmission(dsi, irq_status);
  

•   return IRQ_HANDLED;
  

+} +EXPORT_SYMBOL_GPL(nwl_dsi_irq_handler);

What about placing this function inside nwl-drv.c and make it static?

+int nwl_dsi_enable(struct imx_nwl_dsi *dsi) +{

•   struct device *dev = dsi->dev;
  

•   union phy_configure_opts *phy_cfg = &dsi->phy_cfg;
  

•   int ret;
  

•   if (!dsi->lanes) {
  

•           DRM_DEV_ERROR(dev, "Need DSI lanes: %d\n", dsi->lanes);
  

•           return -EINVAL;
  

•   }
  

•   ret = phy_init(dsi->phy);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to init DSI phy: %d\n", ret);
  

•           return ret;
  

•   }
  

•   ret = phy_configure(dsi->phy, phy_cfg);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
  

•           return ret;
  

•   }
  

•   ret = nwl_dsi_enable_tx_clock(dsi);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to enable tx clock: %d\n", ret);
  

•           return ret;
  

•   }
  

•   ret = nwl_dsi_config_host(dsi);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to set up DSI: %d", ret);
  

•           return ret;
  

•   }
  

•   ret = nwl_dsi_config_dpi(dsi);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to set up DPI: %d", ret);
  

•           return ret;
  

•   }
  

•   ret = phy_power_on(dsi->phy);
  

•   if (ret < 0) {
  

•           DRM_DEV_ERROR(dev, "Failed to power on DPHY (%d)\n", ret);
  

•           return ret;
  

•   }
  

•   nwl_dsi_init_interrupts(dsi);
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(nwl_dsi_enable);

Same here.

+int nwl_dsi_disable(struct imx_nwl_dsi *dsi) +{

•   struct device *dev = dsi->dev;
  

•   DRM_DEV_DEBUG_DRIVER(dev, "Disabling clocks and phy\n");
  

•   phy_power_off(dsi->phy);
  

•   phy_exit(dsi->phy);
  

•   /* Disabling the clock before the phy breaks enabling dsi again */
  

•   clk_disable_unprepare(dsi->tx_esc_clk);
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(nwl_dsi_disable);

Same here.