Hi Laurent, thanks for the review! Most of it seemed clear how to fix for the rest i've put some questions below: On Sat, Jul 27, 2019 at 05:47:00AM +0300, Laurent Pinchart wrote: > Hello Guido, > > Thank you for the patch. > > On Wed, Jul 24, 2019 at 05:52:26PM +0200, Guido Günther wrote: > > This adds initial support for the NWL MIPI DSI Host controller found on > > i.MX8 SoCs. > > > > It adds support for the i.MX8MQ but the same IP can be found on > > e.g. the i.MX8QXP. > > > > It has been tested on the Librem 5 devkit using mxsfb. > > > > Signed-off-by: Guido Günther <agx@xxxxxxxxxxx> > > Co-developed-by: Robert Chiras <robert.chiras@xxxxxxx> > > --- > > drivers/gpu/drm/bridge/Kconfig | 2 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/imx-nwl/Kconfig | 15 + > > drivers/gpu/drm/bridge/imx-nwl/Makefile | 2 + > > drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c | 529 ++++++++++++++++ > > drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h | 72 +++ > > drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c | 745 +++++++++++++++++++++++ > > drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h | 111 ++++ > > 8 files changed, 1477 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/Kconfig > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/Makefile > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c > > create mode 100644 drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index a6eec908c43e..38c3145a7e57 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -152,6 +152,8 @@ source "drivers/gpu/drm/bridge/analogix/Kconfig" > > > > source "drivers/gpu/drm/bridge/adv7511/Kconfig" > > > > +source "drivers/gpu/drm/bridge/imx-nwl/Kconfig" > > + > > As this doesn't seem to be an i.MX-specific IP, I wouldn't use the name > imx in file names or in the code, at least in the parts that are not > NXP-specific. O.k. Since i've not seen other SoCs using this ip core I wasn't sure what would be sharable but we'll figure that out. Renamed to nwl-dsi/ > > source "drivers/gpu/drm/bridge/synopsys/Kconfig" > > > > endmenu > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > index 4934fcf5a6f8..904a9eb3a20a 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -16,4 +16,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > > +obj-y += imx-nwl/ > > obj-y += synopsys/ > > diff --git a/drivers/gpu/drm/bridge/imx-nwl/Kconfig b/drivers/gpu/drm/bridge/imx-nwl/Kconfig > > new file mode 100644 > > index 000000000000..822dba1b380a > > --- /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) > > + depends on COMMON_CLK > > + depends on OF && HAS_IOMEM > > + select DRM_KMS_HELPER > > + select DRM_MIPI_DSI > > + select DRM_PANEL_BRIDGE > > + select GENERIC_PHY_MIPI_DPHY > > + select MFD_SYSCON > > + select REGMAP_MMIO > > + help > > + This enables the Northwest Logic MIPI DSI Host controller as > > + found on NXP's i.MX8 Processors. > > + > > diff --git a/drivers/gpu/drm/bridge/imx-nwl/Makefile b/drivers/gpu/drm/bridge/imx-nwl/Makefile > > new file mode 100644 > > index 000000000000..9fa63483da5b > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx-nwl/Makefile > > @@ -0,0 +1,2 @@ > > +imx-nwl-objs := nwl-drv.o nwl-dsi.o > > +obj-$(CONFIG_DRM_IMX_NWL_DSI) += imx-nwl.o > > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c > > new file mode 100644 > > index 000000000000..451f8f067c6f > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.c > > @@ -0,0 +1,529 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * i.MX8 NWL MIPI DSI host driver > > + * > > + * Copyright (C) 2017 NXP > > + * Copyright (C) 2019 Purism SPC > > + */ > > + > > +#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> > > This doesn't seem to be needed. Dropped. > > > +#include <linux/clk.h> > > +#include <linux/component.h> > > Same here. Dropped (it was a component driver before). > > +#include <linux/gpio/consumer.h> > > +#include <linux/irq.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/mfd/syscon/imx8mq-iomuxc-gpr.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/phy/phy.h> > > +#include <linux/regmap.h> > > +#include <linux/sys_soc.h> > > +#include <video/videomode.h> > > + > > +#include "nwl-drv.h" > > +#include "nwl-dsi.h" > > + > > +#define DRV_NAME "imx-nwl-dsi" > > + > > +/* 8MQ SRC specific registers */ > > +#define SRC_MIPIPHY_RCR 0x28 > > +#define RESET_BYTE_N BIT(1) > > +#define RESET_N BIT(2) > > +#define DPI_RESET_N BIT(3) > > +#define ESC_RESET_N BIT(4) > > +#define PCLK_RESET_N BIT(5) > > + > > +/* Possible clocks */ > > +#define CLK_PIXEL "pixel" > > +#define CLK_CORE "core" > > +#define CLK_BYPASS "bypass" > > + > > +enum imx_ext_regs { > > + IMX_REG_CSR = BIT(1), > > + IMX_REG_SRC = BIT(2), > > + IMX_REG_GPR = BIT(3), > > +}; > > + > > +static const struct regmap_config nwl_dsi_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .max_register = IRQ_MASK2, > > + .name = DRV_NAME, > > +}; > > + > > +struct imx_nwl_platform_data { > > + int (*poweron)(struct imx_nwl_dsi *dsi); > > + int (*poweroff)(struct imx_nwl_dsi *dsi); > > + u32 ext_regs; /* required external registers */ > > + struct imx_nwl_clk_config clk_config[NWL_MAX_PLATFORM_CLOCKS]; > > +}; > > + > > +static inline struct imx_nwl_dsi *bridge_to_dsi(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct imx_nwl_dsi, bridge); > > +} > > + > > +static void imx_nwl_dsi_set_clocks(struct imx_nwl_dsi *dsi, bool enable) > > +{ > > + 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", > > + enable ? "en" : "dis"); > > + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > > + if (!dsi->clk_config[i].present) > > + continue; > > + id = dsi->clk_config[i].id; > > + clk = dsi->clk_config[i].clk; > > + new_rate = dsi->clk_config[i].rate; > > + cur_rate = clk_get_rate(clk); > > + enabled = dsi->clk_config[i].enabled; > > + > > + /* BYPASS clk must have the same rate as PHY_REF clk */ > > + if (!strcmp(id, CLK_BYPASS)) > > + new_rate = clk_get_rate(dsi->phy_ref_clk); > > + > > + if (enable) { > > + if (enabled && new_rate != cur_rate) > > + clk_disable_unprepare(clk); > > + else if (enabled && new_rate == cur_rate) > > + continue; > > + if (new_rate > 0) > > + clk_set_rate(clk, new_rate); > > + ret = clk_prepare_enable(clk); > > + if (ret < 0) { > > + DRM_DEV_ERROR(dev, "Failed to enable clock %s", > > + id); > > + } > > + 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) > > +{ > > + 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); > > +} > > + > > +static void imx_nwl_dsi_disable(struct imx_nwl_dsi *dsi) > > +{ > > + struct device *dev = dsi->dev; > > + > > + if (dsi->dpms_mode != DRM_MODE_DPMS_ON) > > + return; > > + > > The DRM core should guarantee that the bridge won't be disabled twice, > so I don't think you need this check. Similarly I think the enabled flag > in the imx_nwl_clk_config structure can be removed. Dropped and i also simplified the imx_nwl_clk_config - rates are supplied via DT here anyway. > > > + DRM_DEV_DEBUG_DRIVER(dev, "Disabling encoder"); > > Is this really needed ? Dropped. > > + dsi->pdata->poweroff(dsi); > > + imx_nwl_dsi_set_clocks(dsi, false); > > +} > > + > > +static void imx_nwl_select_input_source(struct imx_nwl_dsi *dsi) > > +{ > > + struct device_node *remote; > > + u32 mux_val = IMX8MQ_GPR13_MIPI_MUX_SEL; > > + > > + remote = of_graph_get_remote_node(dsi->dev->of_node, 0, 0); > > + if (strcmp(remote->name, "lcdif") == 0) > > + mux_val = 0; > > + Getting it from dt spares us carrying around more platform specific state. Should i change it anyway? > > Can't you check the remote node at probe time instead of every time the > bridge gets enabled, and program the IO mux accordingly there ? > > This code is i.MX-specific, so it should be isolated in an operation in > struct imx_nwl_platform_data. Done. > > > + DRM_DEV_INFO(dsi->dev, "Using %s as input source\n", > > + (mux_val) ? "DCSS" : "LCDIF"); > > + regmap_update_bits(dsi->mux_sel, IOMUXC_GPR13, > > + IMX8MQ_GPR13_MIPI_MUX_SEL, mux_val); > > + of_node_put(remote); > > +} > > + > > +static void imx_nwl_dsi_bridge_disable(struct drm_bridge *bridge) > > +{ > > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge); > > + > > + if (dsi->dpms_mode != DRM_MODE_DPMS_ON) > > + return; > > + > > + nwl_dsi_disable(dsi); > > + imx_nwl_dsi_disable(dsi); > > + pm_runtime_put_sync(dsi->dev); > > Do you need a put_sync, wouldn't a put do ? Switched to put() only. > > > + dsi->dpms_mode = DRM_MODE_DPMS_OFF; > > +} > > + > > +static bool > > +imx_nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode, > > + struct drm_display_mode *adjusted_mode) > > +{ > > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge); > > + struct device *dev = dsi->dev; > > + union phy_configure_opts new_cfg; > > + unsigned long phy_ref_rate; > > + int ret; > > + > > + ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * If hs clock is unchanged, we're all good - all parameters are > > + * derived from it atm. > > + */ > > + if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate) > > + return true; > > + > > + 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 can't happen. Or are you missing a function call before the check > ? The code used that a long time ago, fixed that path. > > > + 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)); > > The mode_fixup operation shall not change the device state, it can be > called multiple times when trying modes. Moved the parts to 'mode_set' instead. > > > + } > > + > > + /* LCDIF + NWL needs active high sync */ > > + 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; > > return 0; mode_fixup wants a bool but that code is gone anyways. > > > +} > > + > > +static enum drm_mode_status > > +imx_nwl_dsi_bridge_mode_valid(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode) > > +{ > > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge); > > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > + > > + if (bpp < 0) { > > + DRM_DEV_ERROR(dsi->dev, "Invalid pixel format: %d\n", > > + dsi->format); > > + return MODE_BAD; > > + } > > The format isn't part of the mode, so this doesn't belong here. You > should here instead check that the mode clock and other timing data > (especially the visible resolution) are within the range supported by > the device. O.k. I've added a clock check derived from the spec instead. > > + > > + return MODE_OK; > > +} > > + > > +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); > > + pm_runtime_get_sync(dsi->dev); > > + imx_nwl_dsi_enable(dsi); > > + nwl_dsi_enable(dsi); > > + dsi->dpms_mode = DRM_MODE_DPMS_ON; > > +} > > + > > +static int imx_nwl_dsi_bridge_attach(struct drm_bridge *bridge) > > +{ > > + struct imx_nwl_dsi *dsi = bridge->driver_private; > > + struct drm_encoder *encoder = bridge->encoder; > > + > > + if (!encoder) { > > + DRM_DEV_ERROR(dsi->dev, "Parent encoder object not found\n"); > > + return -ENODEV; > > + } > > Can't this happen ? Dropped. > > > + > > + /* Set the encoder type as caller does not know it */ > > + bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI; > > The encoder type is quite meaningless and userspace should not depend on > it, so I wouldn't set it here, especially that the encoder may not > expect the bridge to override its type. Dropped. > > > + > > + /* Attach the panel-bridge to the dsi bridge */ > > + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge); > > +} > > + > > +static void imx_nwl_dsi_bridge_detach(struct drm_bridge *bridge) > > +{ > > + struct imx_nwl_dsi *dsi = bridge->driver_private; > > + > > + drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0); > > This is already done in nwl_dsi_host_detach(). Dropped the whole detach. > > > +} > > + > > +/* see dw-mipi-dsi.c */ > > What for ? :-) Dropped. > > > +static const struct drm_bridge_funcs imx_nwl_dsi_bridge_funcs = { > > + .pre_enable = imx_nwl_dsi_bridge_pre_enable, > > + .disable = imx_nwl_dsi_bridge_disable, > > + .mode_fixup = imx_nwl_dsi_bridge_mode_fixup, > > + .mode_valid = imx_nwl_dsi_bridge_mode_valid, > > + .attach = imx_nwl_dsi_bridge_attach, > > + .detach = imx_nwl_dsi_bridge_detach, > > +}; > > + > > +static int imx_nwl_dsi_parse_dt(struct imx_nwl_dsi *dsi) > > +{ > > + struct device_node *np = dsi->dev->of_node; > > + struct platform_device *pdev = to_platform_device(dsi->dev); > > + struct resource *res; > > + struct clk *clk; > > + const char *clk_id; > > + void __iomem *base; > > + int i, ret; > > + > > + dsi->phy = devm_phy_get(dsi->dev, "dphy"); > > + if (IS_ERR(dsi->phy)) { > > + ret = PTR_ERR(dsi->phy); > > + dev_err(dsi->dev, "Could not get PHY (%d)\n", ret); > > + return ret; > > + } > > + > > + /* Platform dependent clocks */ > > + memcpy(dsi->clk_config, dsi->pdata->clk_config, > > + sizeof(dsi->pdata->clk_config)); > > + > > + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > > + if (!dsi->clk_config[i].present) > > + continue; > > + > > + clk_id = dsi->clk_config[i].id; > > + clk = devm_clk_get(dsi->dev, clk_id); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_err(dsi->dev, "Failed to get %s clock (%d)\n", > > + clk_id, ret); > > + return ret; > > + } > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "Setup clk %s (rate: %lu)\n", > > + clk_id, clk_get_rate(clk)); > > + dsi->clk_config[i].clk = clk; > > + } > > + > > + /* DSI clocks */ > > + clk = devm_clk_get(dsi->dev, "phy_ref"); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_err(dsi->dev, "Failed to get phy_ref clock: %d\n", ret); > > + return ret; > > + } > > + dsi->phy_ref_clk = clk; > > + > > + clk = devm_clk_get(dsi->dev, "rx_esc"); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_err(dsi->dev, "Failed to get rx_esc clock: %d\n", ret); > > + return ret; > > + } > > + dsi->rx_esc_clk = clk; > > + > > + clk = devm_clk_get(dsi->dev, "tx_esc"); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_err(dsi->dev, "Failed to get tx_esc clock: %d\n", ret); > > + return ret; > > + } > > + dsi->tx_esc_clk = clk; > > + > > + 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", > > + ret); > > + return ret; > > + } > > This doesn't seem to be used anywhere. That's true. i had it in for the imx8q*, dropped that (and some other parts) until support for that soc is added. > > > + dsi->reset = syscon_regmap_lookup_by_phandle(np, "src"); > > + if (IS_ERR(dsi->reset) && (dsi->pdata->ext_regs & IMX_REG_SRC)) { > > + ret = PTR_ERR(dsi->reset); > > + DRM_DEV_ERROR(dsi->dev, "Failed to get SRC regmap: %d\n", > > + ret); > > + return ret; > > + } > > Couldn't you model a reset controller in that syscon, and use the reset > controller API here ? It would allow moving the i.MX-specific power on > and off functions from this driver, making it more generic. In fact the reset controller is already there, wired it up accordingly. > > > + dsi->mux_sel = syscon_regmap_lookup_by_phandle(np, "mux-sel"); > > + if (IS_ERR(dsi->mux_sel) && (dsi->pdata->ext_regs & IMX_REG_GPR)) { > > + ret = PTR_ERR(dsi->mux_sel); > > + DRM_DEV_ERROR(dsi->dev, "Failed to get GPR regmap: %d\n", > > + ret); > > + return ret; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + base = devm_ioremap_resource(dsi->dev, res); > > You can replace those two calls with devm_platform_ioremap_resource(). Done. > > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + dsi->regmap = > > + devm_regmap_init_mmio(dsi->dev, base, &nwl_dsi_regmap_config); > > + if (IS_ERR(dsi->regmap)) { > > + ret = PTR_ERR(dsi->regmap); > > + DRM_DEV_ERROR(dsi->dev, > > + "Failed to create NWL DSI regmap: %d\n", ret); > > + return ret; > > + } > > + > > + dsi->irq = platform_get_irq(pdev, 0); > > + if (dsi->irq < 0) { > > + DRM_DEV_ERROR(dsi->dev, "Failed to get device IRQ: %d\n", > > + dsi->irq); > > + return dsi->irq; > > + } > > + > > + return 0; > > +} > > + > > +static int imx8mq_dsi_poweron(struct imx_nwl_dsi *dsi) > > +{ > > + /* otherwise the display stays blank */ > > + usleep_range(200, 300); > > + > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, PCLK_RESET_N, > > + PCLK_RESET_N); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, ESC_RESET_N, > > + ESC_RESET_N); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, RESET_BYTE_N, > > + RESET_BYTE_N); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, DPI_RESET_N, > > + DPI_RESET_N); > > + > > + return 0; > > +} > > + > > +static int imx8mq_dsi_poweroff(struct imx_nwl_dsi *dsi) > > +{ > > + if (USE_SRC_RESET_QUIRK(dsi->quirks)) > > + return 0; > > + > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, PCLK_RESET_N, 0); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, ESC_RESET_N, 0); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, RESET_BYTE_N, 0); > > + regmap_update_bits(dsi->reset, SRC_MIPIPHY_RCR, DPI_RESET_N, 0); > > + return 0; > > +} > > + > > +static const struct drm_bridge_timings imx_nwl_timings = { > > + .input_bus_flags = DRM_BUS_FLAG_DE_LOW, > > +}; > > + > > +static struct imx_nwl_platform_data imx8mq_dev = { > > This structure should be const, especially as it contains function > pointers. Done. > > > + .poweron = &imx8mq_dsi_poweron, > > + .poweroff = &imx8mq_dsi_poweroff, > > + .clk_config = { > > + { .id = CLK_CORE, .present = true }, > > + { .id = CLK_PIXEL, .present = false }, > > + { .id = CLK_BYPASS, .present = false }, > > + }, > > + .ext_regs = IMX_REG_SRC | IMX_REG_GPR, > > +}; > > + > > +static const struct of_device_id imx_nwl_dsi_dt_ids[] = { > > + { .compatible = "fsl,imx8mq-nwl-dsi", .data = &imx8mq_dev, }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, imx_nwl_dsi_dt_ids); > > + > > +static const struct soc_device_attribute imx_nwl_quirks_match[] = { > > + { .soc_id = "i.MX8MQ", .revision = "2.0", > > + .data = (void *)(E11418_HS_MODE_QUIRK | SRC_RESET_QUIRK) }, > > + { /* sentinel. */ }, > > +}; > > + > > +static int imx_nwl_dsi_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + const struct of_device_id *of_id = > > + of_match_device(imx_nwl_dsi_dt_ids, dev); > > + const struct imx_nwl_platform_data *pdata = of_id->data; > > + const struct soc_device_attribute *attr; > > + struct imx_nwl_dsi *dsi; > > + int ret; > > + > > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > > + if (!dsi) > > + return -ENOMEM; > > + > > + dsi->dev = dev; > > + dsi->pdata = pdata; > > + dsi->dpms_mode = DRM_MODE_DPMS_OFF; > > DPMS is legacy, let's not use it within the driver. Removed that since the drm layer keeps the state for us. > > + > > + ret = imx_nwl_dsi_parse_dt(dsi); > > + if (ret) > > + return ret; > > + > > + ret = devm_request_irq(dev, dsi->irq, nwl_dsi_irq_handler, 0, > > + dev_name(dev), dsi); > > + if (ret < 0) { > > + DRM_DEV_ERROR(dev, "Failed to request IRQ: %d (%d)\n", dsi->irq, > > + ret); > > + return ret; > > + } > > + > > + dsi->dsi_host.ops = &nwl_dsi_host_ops; > > + dsi->dsi_host.dev = dev; > > + ret = mipi_dsi_host_register(&dsi->dsi_host); > > + if (ret) { > > + DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret); > > + goto err_cleanup; > > + } > > + > > + attr = soc_device_match(imx_nwl_quirks_match); > > + if (attr) > > + dsi->quirks = (uintptr_t)attr->data; > > + > > + dsi->bridge.driver_private = dsi; > > + dsi->bridge.funcs = &imx_nwl_dsi_bridge_funcs; > > + dsi->bridge.of_node = dev->of_node; > > + dsi->bridge.timings = &imx_nwl_timings; > > + > > + drm_bridge_add(&dsi->bridge); > > + > > + dev_set_drvdata(dev, dsi); > > + pm_runtime_enable(dev); > > + return 0; > > + > > +err_cleanup: > > + devm_free_irq(dev, dsi->irq, dsi); > > + return ret; > > +} > > + > > +static int imx_nwl_dsi_remove(struct platform_device *pdev) > > +{ > > + pm_runtime_disable(&pdev->dev); > > You should call drm_bridge_remove() here, not in > nwl_dsi_host_detach(). I opted to call `mipi_dsi_host_unregister(&dsi->dsi_host)` which would keep the removal in `nwl_dsi_host_detach()` but would also make sure it's called on removal (modelled like cdns-dsi). > > > + return 0; > > +} > > + > > +static struct platform_driver imx_nwl_dsi_driver = { > > + .probe = imx_nwl_dsi_probe, > > + .remove = imx_nwl_dsi_remove, > > + .driver = { > > + .of_match_table = imx_nwl_dsi_dt_ids, > > + .name = DRV_NAME, > > + }, > > +}; > > + > > +module_platform_driver(imx_nwl_dsi_driver); > > + > > +MODULE_AUTHOR("NXP Semiconductor"); > > +MODULE_AUTHOR("Purism SPC"); > > +MODULE_DESCRIPTION("i.MX8 Northwest Logic MIPI-DSI driver"); > > +MODULE_LICENSE("GPL"); /* GPLv2 or later */ > > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h > > new file mode 100644 > > index 000000000000..a1e30c58b627 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-drv.h > > @@ -0,0 +1,72 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * i.MX8 NWL MIPI DSI host driver > > + * > > + * Copyright (C) 2017 NXP > > + * Copyright (C) 2019 Purism SPC > > + */ > > + > > +#ifndef __NWL_DRV_H__ > > +#define __NWL_DRV_H__ > > + > > +#include <drm/drm_mipi_dsi.h> > > +#include <linux/phy/phy.h> > > + > > +struct imx_nwl_platform_data; > > + > > +/* i.MX8 NWL quirks */ > > +/* i.MX8MQ errata E11418 */ > > +#define E11418_HS_MODE_QUIRK BIT(0) > > +#define USE_E11418_HS_MODE_QUIRK(x) ((x) & E11418_HS_MODE_QUIRK) > > + > > +/* Skip DSI bits in SRC on disable to avoid blank display on enable */ > > +#define SRC_RESET_QUIRK BIT(1) > > +#define USE_SRC_RESET_QUIRK(x) ((x) & SRC_RESET_QUIRK) > > The USE_* macros are not shorter to type, so I would type out the & > check explicitly. Dropped. > > + > > +#define NWL_MAX_PLATFORM_CLOCKS 3 > > +struct imx_nwl_clk_config { > > + const char *id; > > + struct clk *clk; > > + bool present; > > + bool enabled; > > + u32 rate; > > +}; > > + > > +struct imx_nwl_dsi { > > + struct drm_bridge bridge; > > + struct mipi_dsi_host dsi_host; > > + struct drm_bridge *panel_bridge; > > + struct device *dev; > > + struct phy *phy; > > + union phy_configure_opts phy_cfg; > > + unsigned int quirks; > > + > > + struct regmap *regmap; > > + int irq; > > + > > + /* External registers */ > > + struct regmap *csr; > > + struct regmap *mux_sel; > > + struct regmap *reset; > > + > > + /* Platform dependent clocks */ > > + struct imx_nwl_clk_config clk_config[3]; > > I would use NWL_MAX_PLATFORM_CLOCKS instead of 3 as > imx_nwl_platform_data uses the macro. That was an omission, fixed. > > > + /* DSI clocks */ > > + struct clk *phy_ref_clk; > > + struct clk *rx_esc_clk; > > + struct clk *tx_esc_clk; > > + > > + /* dsi lanes */ > > + u32 lanes; > > + enum mipi_dsi_pixel_format format; > > + struct videomode vm; > > + unsigned long dsi_mode_flags; > > + > > + int dpms_mode; > > + > > + struct mipi_dsi_transfer *xfer; > > + > > + const struct imx_nwl_platform_data *pdata; > > +}; > > + > > +#endif /* __NWL_DRV_H__ */ > > 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> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_crtc_helper.h> > > +#include <drm/drm_of.h> > > +#include <drm/drm_panel.h> > > +#include <drm/drm_print.h> > > +#include <linux/clk.h> > > +#include <linux/irq.h> > > +#include <linux/regmap.h> > > +#include <video/mipi_display.h> > > +#include <video/videomode.h> > > + > > +#include "nwl-drv.h" > > +#include "nwl-dsi.h" > > + > > +#define MIPI_FIFO_TIMEOUT msecs_to_jiffies(500) > > + > > +/* PKT reg bit manipulation */ > > +#define REG_MASK(e, s) (((1 << ((e) - (s) + 1)) - 1) << (s)) > > +#define REG_PUT(x, e, s) (((x) << (s)) & REG_MASK(e, s)) > > +#define REG_GET(x, e, s) (((x) & REG_MASK(e, s)) >> (s)) > > Let's not reinvent the wheel, linux/bits.h and linux/bitfield.h can be > used instead. Done. > > > + > > +/* > > + * PKT_CONTROL format: > > + * [15: 0] - word count > > + * [17:16] - virtual channel > > + * [23:18] - data type > > + * [24] - LP or HS select (0 - LP, 1 - HS) > > + * [25] - perform BTA after packet is sent > > + * [26] - perform BTA only, no packet tx > > + */ > > +#define WC(x) REG_PUT((x), 15, 0) > > +#define TX_VC(x) REG_PUT((x), 17, 16) > > +#define TX_DT(x) REG_PUT((x), 23, 18) > > +#define HS_SEL(x) REG_PUT((x), 24, 24) > > +#define BTA_TX(x) REG_PUT((x), 25, 25) > > +#define BTA_NO_TX(x) REG_PUT((x), 26, 26) > > + > > +/* > > + * RX_PKT_HEADER format: > > + * [15: 0] - word count > > + * [21:16] - data type > > + * [23:22] - virtual channel > > + */ > > +#define RX_DT(x) REG_GET((x), 21, 16) > > +#define RX_VC(x) REG_GET((x), 23, 22) > > + > > +/* > > + * DSI Video mode > > + */ > > +#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 > > + */ > > +#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 > > + */ > > +#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 }; > > Line breaks please. Done. > > > + > > +struct mipi_dsi_transfer { > > Let's not use such a generic name for a driver-specific structure. You > should name is nwl_dsi_transfer. Fixed. > > > + 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 */ > > +}; > > + > > +static inline int nwl_dsi_write(struct imx_nwl_dsi *dsi, unsigned int reg, > > + 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) > > +{ > > + 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; > > You're loosing the error... Looking at other drivers they often just ignore the return value of regmap_read. The places we use this can't do much to recover from errors so i figured it'd be best to at least log it so it becomes debuggable instead of just dropping it to the floor. > > +} > > + > > +static u32 nwl_dsi_get_dpi_pixel_format(enum mipi_dsi_pixel_format format) > > +{ > > + switch (format) { > > + case MIPI_DSI_FMT_RGB565: > > + return PIXEL_FORMAT_16; > > + case MIPI_DSI_FMT_RGB666: > > + return PIXEL_FORMAT_18L; > > + case MIPI_DSI_FMT_RGB666_PACKED: > > + return PIXEL_FORMAT_18; > > + case MIPI_DSI_FMT_RGB888: > > + return PIXEL_FORMAT_24; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +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); > > No need to export symbols, those fubctions are only meant to be called > from within the same module. Removed (also left overs when i had these in two modules). > > > + > > +#define PSEC_PER_SEC 1000000000000LL > > +/* > > + * ps2bc - Picoseconds to byte clock cycles > > + */ > > +static u32 ps2bc(struct imx_nwl_dsi *dsi, unsigned long long ps) > > +{ > > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > + > > + return DIV_ROUND_UP(ps * dsi->vm.pixelclock * bpp, > > + dsi->lanes * 8 * PSEC_PER_SEC); > > +} > > + > > +/** > > + * 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 > > +/* > > + * us2bc - micro seconds to lp clock cycles > > + */ > > +static u32 us2lp(u32 lp_clk_rate, unsigned long us) > > +{ > > + return DIV_ROUND_UP(us * lp_clk_rate, USEC_PER_SEC); > > +} > > + > > +static int nwl_dsi_config_host(struct imx_nwl_dsi *dsi) > > +{ > > + u32 cycles; > > + struct phy_configure_opts_mipi_dphy *cfg = &dsi->phy_cfg.mipi_dphy; > > + > > + if (dsi->lanes < 1 || dsi->lanes > 4) > > + return -EINVAL; > > + > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "DSI Lanes %d\n", dsi->lanes); > > + nwl_dsi_write(dsi, CFG_NUM_LANES, dsi->lanes - 1); > > + > > + if (dsi->dsi_mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) { > > + nwl_dsi_write(dsi, CFG_NONCONTINUOUS_CLK, 0x01); > > + nwl_dsi_write(dsi, CFG_AUTOINSERT_EOTP, 0x01); > > + } else { > > + nwl_dsi_write(dsi, CFG_NONCONTINUOUS_CLK, 0x00); > > + nwl_dsi_write(dsi, CFG_AUTOINSERT_EOTP, 0x00); > > + } > > + > > + /* values in byte clock cycles */ > > + cycles = ui2bc(dsi, cfg->clk_pre); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles); > > + nwl_dsi_write(dsi, CFG_T_PRE, cycles); > > + cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles); > > + cycles += ui2bc(dsi, cfg->clk_pre); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap: 0x%x\n", cycles); > > + nwl_dsi_write(dsi, CFG_T_POST, cycles); > > + cycles = ps2bc(dsi, cfg->hs_exit); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap: 0x%x\n", cycles); > > + nwl_dsi_write(dsi, CFG_TX_GAP, cycles); > > + > > + nwl_dsi_write(dsi, CFG_EXTRA_CMDS_AFTER_EOTP, 0x01); > > + nwl_dsi_write(dsi, CFG_HTX_TO_COUNT, 0x00); > > + nwl_dsi_write(dsi, CFG_LRX_H_TO_COUNT, 0x00); > > + nwl_dsi_write(dsi, CFG_BTA_H_TO_COUNT, 0x00); > > + /* In LP clock cycles */ > > + cycles = us2lp(cfg->lp_clk_rate, cfg->wakeup); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_twakeup: 0x%x\n", cycles); > > + nwl_dsi_write(dsi, CFG_TWAKEUP, cycles); > > + > > + return 0; > > +} > > + > > +static int nwl_dsi_config_dpi(struct imx_nwl_dsi *dsi) > > +{ > > + struct videomode *vm = &dsi->vm; > > + u32 color_format, mode; > > + bool burst_mode; > > + > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hfront_porch = %d\n", vm->hfront_porch); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hback_porch = %d\n", vm->hback_porch); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hsync_len = %d\n", vm->hsync_len); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "hactive = %d\n", vm->hactive); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vfront_porch = %d\n", vm->vfront_porch); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vback_porch = %d\n", vm->vback_porch); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vsync_len = %d\n", vm->vsync_len); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "vactive = %d\n", vm->vactive); > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "clock = %lu kHz\n", > > + vm->pixelclock / 1000); > > + > > + color_format = nwl_dsi_get_dpi_pixel_format(dsi->format); > > + if (color_format < 0) { > > + DRM_DEV_ERROR(dsi->dev, "Invalid color format 0x%x\n", > > + dsi->format); > > + return color_format; > > + } > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, "pixel fmt = %d\n", dsi->format); > > + > > + nwl_dsi_write(dsi, INTERFACE_COLOR_CODING, DPI_24_BIT); > > + nwl_dsi_write(dsi, PIXEL_FORMAT, color_format); > > + /* > > + * Adjusting input polarity based on the video mode results in > > + * a black screen so always pick active low: > > + */ > > + nwl_dsi_write(dsi, VSYNC_POLARITY, VSYNC_POLARITY_ACTIVE_LOW); > > + nwl_dsi_write(dsi, HSYNC_POLARITY, HSYNC_POLARITY_ACTIVE_LOW); > > + > > + burst_mode = (dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && > > + !(dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE); > > + > > + if (burst_mode) { > > + nwl_dsi_write(dsi, VIDEO_MODE, VIDEO_MODE_BURST_MODE); > > + nwl_dsi_write(dsi, PIXEL_FIFO_SEND_LEVEL, 256); > > + } else { > > + mode = ((dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) ? > > + VIDEO_MODE_BURST_MODE_WITH_SYNC_PULSES : > > + VIDEO_MODE_NON_BURST_MODE_WITH_SYNC_EVENTS); > > + nwl_dsi_write(dsi, VIDEO_MODE, mode); > > + nwl_dsi_write(dsi, PIXEL_FIFO_SEND_LEVEL, vm->hactive); > > + } > > + > > + nwl_dsi_write(dsi, HFP, vm->hfront_porch); > > + nwl_dsi_write(dsi, HBP, vm->hback_porch); > > + nwl_dsi_write(dsi, HSA, vm->hsync_len); > > + > > + nwl_dsi_write(dsi, ENABLE_MULT_PKTS, 0x0); > > + nwl_dsi_write(dsi, BLLP_MODE, 0x1); > > + nwl_dsi_write(dsi, ENABLE_MULT_PKTS, 0x0); > > + nwl_dsi_write(dsi, USE_NULL_PKT_BLLP, 0x0); > > + nwl_dsi_write(dsi, VC, 0x0); > > + > > + nwl_dsi_write(dsi, PIXEL_PAYLOAD_SIZE, vm->hactive); > > + nwl_dsi_write(dsi, VACTIVE, vm->vactive - 1); > > + nwl_dsi_write(dsi, VBP, vm->vback_porch); > > + nwl_dsi_write(dsi, VFP, vm->vfront_porch); > > + > > + return 0; > > +} > > + > > +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; > > +} > > + > > +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; > > +} > > + > > +static void nwl_dsi_init_interrupts(struct imx_nwl_dsi *dsi) > > +{ > > + u32 irq_enable; > > + > > + nwl_dsi_write(dsi, IRQ_MASK, 0xffffffff); > > + nwl_dsi_write(dsi, IRQ_MASK2, 0x7); > > + > > + irq_enable = ~(u32)(TX_PKT_DONE_MASK | RX_PKT_HDR_RCVD_MASK | > > + TX_FIFO_OVFLW_MASK | HS_TX_TIMEOUT_MASK); > > + > > + nwl_dsi_write(dsi, IRQ_MASK, irq_enable); > > +} > > + > > +static int nwl_dsi_host_attach(struct mipi_dsi_host *dsi_host, > > + struct mipi_dsi_device *device) > > +{ > > + struct imx_nwl_dsi *dsi = > > + container_of(dsi_host, struct imx_nwl_dsi, dsi_host); > > + struct device *dev = dsi->dev; > > + struct drm_bridge *bridge; > > + struct drm_panel *panel; > > + int ret; > > + > > + DRM_DEV_INFO(dev, "lanes=%u, format=0x%x flags=0x%lx\n", device->lanes, > > + device->format, device->mode_flags); > > + > > + if (device->lanes < 1 || device->lanes > 4) > > + return -EINVAL; > > + > > + dsi->lanes = device->lanes; > > + dsi->format = device->format; > > + dsi->dsi_mode_flags = device->mode_flags; > > + > > + ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel, > > + &bridge); > > + if (ret) > > + return ret; > > + > > + if (panel) { > > + bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI); > > + if (IS_ERR(bridge)) > > + return PTR_ERR(bridge); > > + } > > + > > + dsi->panel_bridge = bridge; > > + drm_bridge_add(&dsi->bridge); > > + > > + return 0; > > +} > > + > > +static int nwl_dsi_host_detach(struct mipi_dsi_host *dsi_host, > > + struct mipi_dsi_device *device) > > +{ > > + struct imx_nwl_dsi *dsi = > > + container_of(dsi_host, struct imx_nwl_dsi, dsi_host); > > + > > + drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0); > > + drm_bridge_remove(&dsi->bridge); > > + > > + return 0; > > +} > > + > > +static bool nwl_dsi_read_packet(struct imx_nwl_dsi *dsi, u32 status) > > +{ > > + struct device *dev = dsi->dev; > > + struct mipi_dsi_transfer *xfer = dsi->xfer; > > + u8 *payload = xfer->msg->rx_buf; > > + u32 val; > > + u16 word_count; > > + u8 channel; > > + u8 data_type; > > + > > + xfer->status = 0; > > + > > + if (xfer->rx_word_count == 0) { > > + if (!(status & RX_PKT_HDR_RCVD)) > > + return false; > > + /* Get the RX header and parse it */ > > + val = nwl_dsi_read(dsi, RX_PKT_HEADER); > > + word_count = WC(val); > > + channel = RX_VC(val); > > + data_type = RX_DT(val); > > + > > + if (channel != xfer->msg->channel) { > > + DRM_DEV_ERROR(dev, > > + "[%02X] Channel mismatch (%u != %u)\n", > > + xfer->cmd, channel, xfer->msg->channel); > > + return true; > > + } > > + > > + switch (data_type) { > > + case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_2BYTE: > > + /* Fall through */ > > + case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_2BYTE: > > + if (xfer->msg->rx_len > 1) { > > + /* read second byte */ > > + payload[1] = word_count >> 8; > > + ++xfer->rx_len; > > + } > > + /* Fall through */ > > + case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_1BYTE: > > + /* Fall through */ > > + case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_1BYTE: > > + if (xfer->msg->rx_len > 0) { > > + /* read first byte */ > > + payload[0] = word_count & 0xff; > > + ++xfer->rx_len; > > + } > > + xfer->status = xfer->rx_len; > > + return true; > > + case MIPI_DSI_RX_ACKNOWLEDGE_AND_ERROR_REPORT: > > + word_count &= 0xff; > > + DRM_DEV_ERROR(dev, "[%02X] DSI error report: 0x%02x\n", > > + xfer->cmd, word_count); > > + xfer->status = -EPROTO; > > + return true; > > + } > > + > > + if (word_count > xfer->msg->rx_len) { > > + DRM_DEV_ERROR( > > + dev, > > + "[%02X] Receive buffer too small: %lu (< %u)\n", > > + xfer->cmd, xfer->msg->rx_len, word_count); > > + return true; > > + } > > + > > + xfer->rx_word_count = word_count; > > + } else { > > + /* Set word_count from previous header read */ > > + word_count = xfer->rx_word_count; > > + } > > + > > + /* If RX payload is not yet received, wait for it */ > > + if (!(status & RX_PKT_PAYLOAD_DATA_RCVD)) > > + return false; > > + > > + /* Read the RX payload */ > > + while (word_count >= 4) { > > + val = nwl_dsi_read(dsi, RX_PAYLOAD); > > + payload[0] = (val >> 0) & 0xff; > > + payload[1] = (val >> 8) & 0xff; > > + payload[2] = (val >> 16) & 0xff; > > + payload[3] = (val >> 24) & 0xff; > > + payload += 4; > > + xfer->rx_len += 4; > > + word_count -= 4; > > + } > > + > > + if (word_count > 0) { > > + val = nwl_dsi_read(dsi, RX_PAYLOAD); > > + switch (word_count) { > > + case 3: > > + payload[2] = (val >> 16) & 0xff; > > + ++xfer->rx_len; > > + /* Fall through */ > > + case 2: > > + payload[1] = (val >> 8) & 0xff; > > + ++xfer->rx_len; > > + /* Fall through */ > > + case 1: > > + payload[0] = (val >> 0) & 0xff; > > + ++xfer->rx_len; > > + break; > > + } > > + } > > + > > + xfer->status = xfer->rx_len; > > + > > + return true; > > +} > > + > > +static void nwl_dsi_finish_transmission(struct imx_nwl_dsi *dsi, u32 status) > > +{ > > + struct mipi_dsi_transfer *xfer = dsi->xfer; > > + bool end_packet = false; > > + > > + if (!xfer) > > + return; > > + > > + if (status & TX_FIFO_OVFLW) { > > + DRM_DEV_ERROR_RATELIMITED(dsi->dev, "tx fifo overflow\n"); > > + return; > > + } > > + > > + if (status & HS_TX_TIMEOUT) { > > + DRM_DEV_ERROR_RATELIMITED(dsi->dev, "HS tx timeout\n"); > > + return; > > + } > > + > > + if (xfer->direction == DSI_PACKET_SEND && status & TX_PKT_DONE) { > > + xfer->status = xfer->tx_len; > > + end_packet = true; > > + } else if (status & DPHY_DIRECTION && > > + ((status & (RX_PKT_HDR_RCVD | RX_PKT_PAYLOAD_DATA_RCVD)))) { > > + end_packet = nwl_dsi_read_packet(dsi, status); > > + } > > + > > + if (end_packet) > > + complete(&xfer->completed); > > +} > > + > > +static void nwl_dsi_begin_transmission(struct imx_nwl_dsi *dsi) > > +{ > > + struct mipi_dsi_transfer *xfer = dsi->xfer; > > + struct mipi_dsi_packet *pkt = &xfer->packet; > > + const u8 *payload; > > + size_t length; > > + u16 word_count; > > + u8 hs_mode; > > + u32 val; > > + u32 hs_workaround = 0; > > + > > + /* Send the payload, if any */ > > + length = pkt->payload_length; > > + payload = pkt->payload; > > + > > + while (length >= 4) { > > + val = get_unaligned_le32(payload); > > The framework doesn't guarantee the payload to be aligned on a multiple > of 4 bytes ? It does as far as i can tell, dropped. > > > + hs_workaround |= !(val & 0xFFFF00); > > + nwl_dsi_write(dsi, TX_PAYLOAD, val); > > + payload += 4; > > + length -= 4; > > + } > > + /* Send the rest of the payload */ > > + val = 0; > > + switch (length) { > > + case 3: > > + val |= payload[2] << 16; > > + /* Fall through */ > > + case 2: > > + val |= payload[1] << 8; > > + hs_workaround |= !(val & 0xFFFF00); > > + /* Fall through */ > > + case 1: > > + val |= payload[0]; > > + nwl_dsi_write(dsi, TX_PAYLOAD, val); > > + break; > > + } > > + xfer->tx_len = pkt->payload_length; > > + > > + /* > > + * Send the header > > + * header[0] = Virtual Channel + Data Type > > + * header[1] = Word Count LSB (LP) or first param (SP) > > + * header[2] = Word Count MSB (LP) or second param (SP) > > + */ > > + word_count = pkt->header[1] | (pkt->header[2] << 8); > > + if ((hs_workaround && USE_E11418_HS_MODE_QUIRK(dsi->quirks))) { > > + DRM_DEV_DEBUG_DRIVER(dsi->dev, > > + "Using hs mode workaround for cmd 0x%x\n", > > + xfer->cmd); > > + hs_mode = 1; > > + } else { > > + hs_mode = (xfer->msg->flags & MIPI_DSI_MSG_USE_LPM) ? 0 : 1; > > + } > > + val = WC(word_count) | > > + TX_VC(xfer->msg->channel) | > > + TX_DT(xfer->msg->type) | > > + HS_SEL(hs_mode) | > > + BTA_TX(xfer->need_bta); > > + nwl_dsi_write(dsi, PKT_CONTROL, val); > > + > > + /* Send packet command */ > > + nwl_dsi_write(dsi, SEND_PACKET, 0x1); > > +} > > + > > +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); > > + > > + /* Initiate the DSI packet transmision */ > > + nwl_dsi_begin_transmission(dsi); > > + > > + if (!wait_for_completion_timeout(&xfer.completed, MIPI_FIFO_TIMEOUT)) { > > + DRM_DEV_ERROR(dsi_host->dev, "[%02X] DSI transfer timed out\n", > > + xfer.cmd); > > + ret = -ETIMEDOUT; > > + } else { > > + ret = xfer.status; > > + } > > + > > + clk_disable_unprepare(dsi->rx_esc_clk); > > + > > + return ret; > > +} > > + > > +const struct mipi_dsi_host_ops nwl_dsi_host_ops = { > > + .attach = nwl_dsi_host_attach, > > + .detach = nwl_dsi_host_detach, > > + .transfer = nwl_dsi_host_transfer, > > +}; > > + > > +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); > > + > > +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); > > + > > +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); > > diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h > > new file mode 100644 > > index 000000000000..7bcf804843e2 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.h > > @@ -0,0 +1,111 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * i.MX8 NWL MIPI DSI host driver > > + * > > + * Copyright (C) 2017 NXP > > + * Copyright (C) 2019 Purism SPC > > + */ > > +#ifndef __NWL_DSI_H__ > > +#define __NWL_DSI_H__ > > + > > +#include <drm/drm_mipi_dsi.h> > > + > > +/* DSI HOST registers */ > > +#define CFG_NUM_LANES 0x0 > > Some of the register names are quite prone to namespace clashes. I > recommend prefixing them all with NWL_DSI_. Done, also the defines in nwl-dsi.c. > > > +#define CFG_NONCONTINUOUS_CLK 0x4 > > +#define CFG_T_PRE 0x8 > > +#define CFG_T_POST 0xc > > +#define CFG_TX_GAP 0x10 > > +#define CFG_AUTOINSERT_EOTP 0x14 > > +#define CFG_EXTRA_CMDS_AFTER_EOTP 0x18 > > +#define CFG_HTX_TO_COUNT 0x1c > > +#define CFG_LRX_H_TO_COUNT 0x20 > > +#define CFG_BTA_H_TO_COUNT 0x24 > > +#define CFG_TWAKEUP 0x28 > > +#define CFG_STATUS_OUT 0x2c > > +#define RX_ERROR_STATUS 0x30 > > + > > +/* DSI DPI registers */ > > +#define PIXEL_PAYLOAD_SIZE 0x200 > > +#define PIXEL_FIFO_SEND_LEVEL 0x204 > > +#define INTERFACE_COLOR_CODING 0x208 > > +#define PIXEL_FORMAT 0x20c > > +#define VSYNC_POLARITY 0x210 > > +#define VSYNC_POLARITY_ACTIVE_LOW 0 > > +#define VSYNC_POLARITY_ACTIVE_HIGH BIT(1) > > + > > +#define HSYNC_POLARITY 0x214 > > +#define HSYNC_POLARITY_ACTIVE_LOW 0 > > +#define HSYNC_POLARITY_ACTIVE_HIGH BIT(1) > > + > > +#define VIDEO_MODE 0x218 > > +#define HFP 0x21c > > +#define HBP 0x220 > > +#define HSA 0x224 > > +#define ENABLE_MULT_PKTS 0x228 > > +#define VBP 0x22c > > +#define VFP 0x230 > > +#define BLLP_MODE 0x234 > > +#define USE_NULL_PKT_BLLP 0x238 > > +#define VACTIVE 0x23c > > +#define VC 0x240 > > + > > +/* DSI APB PKT control */ > > +#define TX_PAYLOAD 0x280 > > +#define PKT_CONTROL 0x284 > > +#define SEND_PACKET 0x288 > > +#define PKT_STATUS 0x28c > > +#define PKT_FIFO_WR_LEVEL 0x290 > > +#define PKT_FIFO_RD_LEVEL 0x294 > > +#define RX_PAYLOAD 0x298 > > +#define RX_PKT_HEADER 0x29c > > + > > +/* DSI IRQ handling */ > > +#define IRQ_STATUS 0x2a0 > > +#define SM_NOT_IDLE BIT(0) > > +#define TX_PKT_DONE BIT(1) > > +#define DPHY_DIRECTION BIT(2) > > +#define TX_FIFO_OVFLW BIT(3) > > +#define TX_FIFO_UDFLW BIT(4) > > +#define RX_FIFO_OVFLW BIT(5) > > +#define RX_FIFO_UDFLW BIT(6) > > +#define RX_PKT_HDR_RCVD BIT(7) > > +#define RX_PKT_PAYLOAD_DATA_RCVD BIT(8) > > +#define BTA_TIMEOUT BIT(29) > > +#define LP_RX_TIMEOUT BIT(30) > > +#define HS_TX_TIMEOUT BIT(31) > > + > > +#define IRQ_STATUS2 0x2a4 > > +#define SINGLE_BIT_ECC_ERR BIT(0) > > +#define MULTI_BIT_ECC_ERR BIT(1) > > +#define CRC_ERR BIT(2) > > + > > +#define IRQ_MASK 0x2a8 > > +#define SM_NOT_IDLE_MASK BIT(0) > > +#define TX_PKT_DONE_MASK BIT(1) > > +#define DPHY_DIRECTION_MASK BIT(2) > > +#define TX_FIFO_OVFLW_MASK BIT(3) > > +#define TX_FIFO_UDFLW_MASK BIT(4) > > +#define RX_FIFO_OVFLW_MASK BIT(5) > > +#define RX_FIFO_UDFLW_MASK BIT(6) > > +#define RX_PKT_HDR_RCVD_MASK BIT(7) > > +#define RX_PKT_PAYLOAD_DATA_RCVD_MASK BIT(8) > > +#define BTA_TIMEOUT_MASK BIT(29) > > +#define LP_RX_TIMEOUT_MASK BIT(30) > > +#define HS_TX_TIMEOUT_MASK BIT(31) > > + > > +#define IRQ_MASK2 0x2ac > > +#define SINGLE_BIT_ECC_ERR_MASK BIT(0) > > +#define MULTI_BIT_ECC_ERR_MASK BIT(1) > > +#define CRC_ERR_MASK BIT(2) > > + > > +extern const struct mipi_dsi_host_ops nwl_dsi_host_ops; > > + > > +irqreturn_t nwl_dsi_irq_handler(int irq, void *data); > > +int nwl_dsi_enable(struct imx_nwl_dsi *dsi); > > +int nwl_dsi_disable(struct imx_nwl_dsi *dsi); > > +int nwl_dsi_get_dphy_params(struct imx_nwl_dsi *dsi, > > + const struct drm_display_mode *mode, > > + union phy_configure_opts *phy_opts); > > + > > +#endif /* __NWL_DSI_H__ */ > > -- > Regards, > > Laurent Pinchart > Cheers and thanks for having a look! -- Guido _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel