Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
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 > > Co-developed-by: Robert Chiras > > --- > > 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 ..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 ..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 ..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 > > +#include > > +#include > > +#include > > +#include > > +#include > > This doesn't seem to be needed. Dropped. > > > +#include > > +#include > > Same here. Dropped (it was a component driver before). > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "nwl-drv.h" > > +#include "nwl-dsi.h" > > + > > +#define DRV_NAME "imx-nwl-dsi" > > + > > +/* 8MQ SRC specific registers */ > > +#define SRC_MIPIPHY_RCR0x28 > > +#define RESET_BYTE_N
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
On Wed, Jul 31, 2019 at 1:40 PM Jernej Škrabec wrote: > > Yes, I understood the idea, but this would print: > > > > ensabling or dissabling :-) > > No, it wouldn't. That extra "s" is part of "%s", e.g. part of format > specifier. Ops, you are right. Sorry about that! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi, On Wed, Jul 31, 2019 at 11:43:47AM -0300, Fabio Estevam wrote: > Hi Guido, > > On Wed, Jul 31, 2019 at 11:35 AM Guido Günther wrote: > > > The idea is to have > > > > "%sabling platform clocks", enable ? "en" : "dis"); > > > > depending whether clocks are enabled/disabled. > > Yes, I understood the idea, but this would print: > > ensabling or dissabling :-) The 's' is from the '%s' format specifier: [ 2610.804174] imx-nwl-dsi 30a0.mipi_dsi: [drm:imx_nwl_dsi_bridge_disable] disabling platform clocks [ 2710.996279] imx-nwl-dsi 30a0.mipi_dsi: [drm:imx_nwl_dsi_bridge_pre_enable] enabling platform clocks I'll change that to use the full strings though since i also had to look twice to be sure there's no double 's'. > > > > Same here. Please return 'int' instead. > > > > This is from drm_bridge_funcs so the prototype is fixed. I'm not sure > > how what's the best way to bubble up fatal errors through the drm layer? > > Ok, so let's not change this one. > > > I went for DRM_DEV_ERROR() since that what i used in the rest of the > > driver and these ones were omission. Hope that's o.k. > > No strong preferences here. I just think dev_err() easier to type and shorter. > > Thanks for this work! Thanks again for having a look! -- Guido
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi! Dne sreda, 31. julij 2019 ob 16:43:47 CEST je Fabio Estevam napisal(a): > Hi Guido, > > On Wed, Jul 31, 2019 at 11:35 AM Guido Günther wrote: > > The idea is to have > > > > "%sabling platform clocks", enable ? "en" : "dis"); > > > > depending whether clocks are enabled/disabled. > > Yes, I understood the idea, but this would print: > > ensabling or dissabling :-) No, it wouldn't. That extra "s" is part of "%s", e.g. part of format specifier. Best regards, Jernej > > > > Same here. Please return 'int' instead. > > > > This is from drm_bridge_funcs so the prototype is fixed. I'm not sure > > how what's the best way to bubble up fatal errors through the drm layer? > > Ok, so let's not change this one. > > > I went for DRM_DEV_ERROR() since that what i used in the rest of the > > driver and these ones were omission. Hope that's o.k. > > No strong preferences here. I just think dev_err() easier to type and > shorter. > > Thanks for this work!
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi Guido, On Wed, Jul 31, 2019 at 11:35 AM Guido Günther wrote: > The idea is to have > > "%sabling platform clocks", enable ? "en" : "dis"); > > depending whether clocks are enabled/disabled. Yes, I understood the idea, but this would print: ensabling or dissabling :-) > > Same here. Please return 'int' instead. > > This is from drm_bridge_funcs so the prototype is fixed. I'm not sure > how what's the best way to bubble up fatal errors through the drm layer? Ok, so let's not change this one. > I went for DRM_DEV_ERROR() since that what i used in the rest of the > driver and these ones were omission. Hope that's o.k. No strong preferences here. I just think dev_err() easier to type and shorter. Thanks for this work! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi, On Sat, Jul 27, 2019 at 05:04:44AM +0300, Laurent Pinchart wrote: > Hello, > > On Fri, Jul 26, 2019 at 05:01:52PM -0300, Fabio Estevam wrote: > > Hi Guido, > > > > Thanks for your work on this driver! > > > > On Wed, Jul 24, 2019 at 12:52 PM Guido Günther 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. > > I'd go even further and not use the prefix imx in the driver name or > anywhere in the code. My idea was to do that when another possible user comes up to see what can be shared but I can do that for v2. Cheers, -- Guido > > [snip] > > -- > Regards, > > Laurent Pinchart >
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi Fabio,
thanks for having a look! I followed most of your comments, there's some
things i'm unsure, see below:
On Fri, Jul 26, 2019 at 05:01:52PM -0300, Fabio Estevam wrote:
> Hi Guido,
>
> Thanks for your work on this driver!
>
> On Wed, Jul 24, 2019 at 12:52 PM Guido Günther 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.
Done.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> I did not find gpio AP used in this driver.
Dropped the include.
>
> > +static void imx_nwl_dsi_set_clocks(struct imx_nwl_dsi *dsi, bool enable)
>
> Better make it to return 'int' instead...
Done, but see below.
>
> > +{
> > + 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'.
The idea is to have
"%sabling platform clocks", enable ? "en" : "dis");
depending whether clocks are enabled/disabled.
>
> > +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.
done.
>
> > + }
> > + 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.
This is from drm_bridge_funcs so the prototype is fixed. I'm not sure
how what's the best way to bubble up fatal errors through the drm layer?
> > +{
> > + 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.
If fixed the return type here as well but this will again get lost in
`_pre_enable` which again is void in drm_bridge_funcs (see below).
>
> > + 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.
Fixed.
>
> > + 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(>phy_cfg, _cfg, sizeof(new_cfg));
> > + }
> > +
> > + /* LCDIF + NWL needs active high sync */
>
> Would this still work if DCSS is used instead?
I'll check once there's a DCSS driver that can drive this. The current
vendor one is bound to imx-displays-subsystem and cant' drive a bridge.
>
> > + 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, >vm);
> > + drm_mode_debug_printmodeline(adjusted_mode);
> > +
> > + return ret == 0;
>
> At this point ret is always 0.
Right, there was some more logic in there that got removed. Changed to
always signal success.
>
> > +static void imx_nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
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
> Co-developed-by: Robert Chiras
> ---
> 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.
> 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 ..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 ..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 ..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
> +#include
> +#include
> +#include
> +#include
> +#include
This doesn't seem to be needed.
> +#include
> +#include
Same here.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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 {
> +
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hello, On Fri, Jul 26, 2019 at 05:01:52PM -0300, Fabio Estevam wrote: > Hi Guido, > > Thanks for your work on this driver! > > On Wed, Jul 24, 2019 at 12:52 PM Guido Günther 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. I'd go even further and not use the prefix imx in the driver name or anywhere in the code. [snip] -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
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(>phy_cfg, _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, >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:
> +
Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
Hi Guido.
Following some trivial comments.
As for the overall design I already commented on that in the binding.
(bridge versus display controller)
That it can work on top of mxsfb is a good indication that it is a
bridge but I just do not see the full picture.
In general the code looked clean and neat.
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.
Looking at mxsfb I wonder hw this was done, as there seems to be no
bridge support in mxsfb. Using a patched version of mxsfb?
> 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-$(ONFIG_DRM_IMX_NWL_DSI) += imx-nwl/?
So we do not visit the directory unless required.
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx-nwl/Makefile
> @@ -0,0 +1,2 @@
> +imx-nwl-objs := nwl-drv.o nwl-dsi.o
The preferred syntax is
imx-nwl-y := nwl-drv.o nwl-dsi.o
See for example Makefile for mxsfb.
Consider to introduce
header-test-y += nwl-drv.h nwl-dsi.h
So we at build time check that the headers are self-contained.
(they include what they need).
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "nwl-drv.h"
> +#include "nwl-dsi.h"
The most typical order of include files are:
#include
#include
#include
#include ""
With the empty lines in-between each block.
And sorted like is already done here.
This in general for all the files for this driver.
> +
> +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, _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) {
> + 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(>phy_cfg, _cfg, sizeof(new_cfg));
> + }
> +
> + /* 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, >vm);
Hmm, the videomode is just another representation of data already
included in display_mode.
And, as a personal itch, I consider videomode as something that belongs
in the old fb drivers, and not drm drivers. But that may be me only.
Sam
[PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support
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
Co-developed-by: Robert Chiras
---
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"
+
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 ..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 ..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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "nwl-drv.h"
+#include "nwl-dsi.h"
+
+#define DRV_NAME "imx-nwl-dsi"
+
+/* 8MQ SRC specific registers */
+#define SRC_MIPIPHY_RCR0x28
+#define RESET_BYTE_N BIT(1)
+#define RESET_NBIT(2)
+#define DPI_RESET_NBIT(3)
+#define ESC_RESET_NBIT(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;
+
