[PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver
Hi Russell,
On 12/17/2014 06:40 PM, Russell King - ARM Linux wrote:
> On Wed, Dec 17, 2014 at 05:44:33PM +0800, Liu Ying wrote:
>> Hi Thierry,
>>
>> Sorry for the late response.
>> I tried to address almost all your comments locally first.
>> More feedback below.
>>
>> On 12/10/2014 09:16 PM, Thierry Reding wrote:
>>> On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote:
+static int check_status(struct imx_mipi_dsi *dsi, u32 reg, u32 status,
+ int timeout, bool to_set)
+{
+ u32 val;
+ bool out = false;
+
+ val = dsi_read(dsi, reg);
+ for (;;) {
+ out = to_set ? (val & status) : !(val & status);
+ if (out)
+ break;
+
+ if (!timeout--)
+ return -EFAULT;
+
+ msleep(1);
+ val = dsi_read(dsi, reg);
+ }
+ return 0;
+}
>>>
>>> You should probably use a properly timed loop here. msleep() isn't
>>> guaranteed to return after exactly one millisecond, so your timeout is
>>> never going to be accurate. Something like the following would be better
>>> in my opinion:
>>>
>>> timeout = jiffies + msecs_to_jiffies(timeout);
>>>
>>> while (time_before(jiffies, timeout)) {
>>> ...
>>> }
>>>
>>> Also timeout should be unsigned long in that case.
>>
>> Accepted.
>
> Actually, that's a bad example: what we want to do is to assess success
> after we wait, before we decide that something has failed. In other
> words, we don't want to wait, and decide that we failed without first
> checking for success.
>
> In any case, returning -EFAULT is not sane: EFAULT doesn't mean "fault"
> it means "Bad address", and it is returned to userspace to mean that
> userspace passed the kernel a bad address. That definition does /not/
> fit what's going on here.
>
> timeout = jiffies + msecs_to_jiffies(timeout);
>
> do {
> val = dsi_read(dsi, reg);
> out = to_set ? (val & status) : !(val & status);
> if (out)
> break;
>
> if (time_is_after_jiffies(timeout))
time_is_after_jiffies(a) is defined as time_before(jiffies, a).
So, this line should be changed to
if (time_after(jiffies, timeout))
Right?
> return -ETIMEDOUT;
>
> msleep(1);
> } while (1);
>
> return 0;
>
> would be better: we only fail immediately after we have checked whether
> we succeeded, and we also do the first check immediately.
>
Does this one look better? I use cpu_relax() instead of msleep(1).
expire = jiffies + msecs_to_jiffies(timeout);
for (;;) {
val = dsi_read(dsi, reg);
out = to_set ? (val & status) : !(val & status);
if (out)
break;
if (time_after(jiffies, expire))
return -ETIMEDOUT;
cpu_relax();
}
return 0;
Regards,
Liu Ying
[PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver
Hi Thierry, Sorry for the late response. I tried to address almost all your comments locally first. More feedback below. On 12/10/2014 09:16 PM, Thierry Reding wrote: > On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote: >> This patch adds i.MX MIPI DSI host controller driver support. >> Currently, the driver supports the burst with sync pulses mode only. >> >> Signed-off-by: Liu Ying >> --- >> .../devicetree/bindings/drm/imx/mipi_dsi.txt | 81 ++ >> drivers/gpu/drm/imx/Kconfig|6 + >> drivers/gpu/drm/imx/Makefile |1 + >> drivers/gpu/drm/imx/imx-mipi-dsi.c | 1017 >> >> 4 files changed, 1105 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt >> create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c >> >> diff --git a/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt >> b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt >> new file mode 100644 >> index 000..3d07fd7 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt >> @@ -0,0 +1,81 @@ >> +Device-Tree bindings for MIPI DSI host controller >> + >> +MIPI DSI host controller >> + >> + >> +The MIPI DSI host controller is a Synopsys DesignWare IP. >> +It is a digital core that implements all protocol functions defined >> +in the MIPI DSI specification, providing an interface between the >> +system and the MIPI DPHY, and allowing communication with a MIPI DSI >> +compliant display. >> + >> +Required properties: >> + - #address-cells : Should be <1>. >> + - #size-cells : Should be <0>. >> + - compatible : Should be "fsl,imx6q-mipi-dsi" for i.MX6q/sdl SoCs. >> + - reg : Physical base address of the controller and length of memory >> + mapped region. >> + - interrupts : The controller's interrupt number to the CPU(s). >> + - gpr : Should be <>. >> + The phandle points to the iomuxc-gpr region containing the >> + multiplexer control register for the controller. > > Side-note: Shouldn't this really be a pinmux, then? No. The muxing is inside the i.MX SoC. There is a DT binding documentation for the system controller node(gpr) at [1]. And, for i.MX DT sources, there are several existing use cases in which the gpr node is referred by other nodes. [1] Documentation/devicetree/bindings/mfd/syscon.txt. > >> + - clocks, clock-names : Phandles to the controller pllref, pllref_gate >> + and core_cfg clocks, as described in [1] and [2]. >> + - panel at 0 : A panel node which contains a display-timings child node as >> + defined in [3]. > > There's no need for these to be named panel@*. They could be bridges for > example. And no, they shouldn't contain a display-timings child node > either. Panels should have a proper driver and the driver being device > specific it should have the timings embedded. Ok, I'll move the timing to the panel driver. > >> + - port@[0-4] : Up to four port nodes with endpoint definitions as defined >> + in [4], corresponding to the four inputs to the controller multiplexer. >> + Note that each port node should contain the input-port property to >> + distinguish it from the panel node, as described in [5]. > > [4] says that you can group all port nodes under a ports parent node. I > think this is really what you want to do here to make it clear that the > ports aren't part of the DSI host binding part of the device. > Accepted. >> diff --git a/drivers/gpu/drm/imx/imx-mipi-dsi.c >> b/drivers/gpu/drm/imx/imx-mipi-dsi.c > [...] >> +/* >> + * i.MX drm driver - MIPI DSI Host Controller >> + * >> + * Copyright (C) 2011-2014 Freescale Semiconductor, Inc. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Don't you want the more generic linux/math64.h here? I'll use linux/math64.h. > >> +#include >> +#include >> +#include > > I don't see any of the functions defined in that header used here. I'll remove this. > >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "imx-drm.h" >> + >> +#define DRIVER_NAME "imx-mipi-dsi" >> + >> +#define DSI_VERSION 0x00 >> + >> +#define DSI_PWR_UP 0x04 >> +#define RESET
[PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver
On Wed, Dec 17, 2014 at 05:44:33PM +0800, Liu Ying wrote:
> Hi Thierry,
>
> Sorry for the late response.
> I tried to address almost all your comments locally first.
> More feedback below.
>
> On 12/10/2014 09:16 PM, Thierry Reding wrote:
> >On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote:
> >>+static int check_status(struct imx_mipi_dsi *dsi, u32 reg, u32 status,
> >>+ int timeout, bool to_set)
> >>+{
> >>+ u32 val;
> >>+ bool out = false;
> >>+
> >>+ val = dsi_read(dsi, reg);
> >>+ for (;;) {
> >>+ out = to_set ? (val & status) : !(val & status);
> >>+ if (out)
> >>+ break;
> >>+
> >>+ if (!timeout--)
> >>+ return -EFAULT;
> >>+
> >>+ msleep(1);
> >>+ val = dsi_read(dsi, reg);
> >>+ }
> >>+ return 0;
> >>+}
> >
> >You should probably use a properly timed loop here. msleep() isn't
> >guaranteed to return after exactly one millisecond, so your timeout is
> >never going to be accurate. Something like the following would be better
> >in my opinion:
> >
> > timeout = jiffies + msecs_to_jiffies(timeout);
> >
> > while (time_before(jiffies, timeout)) {
> > ...
> > }
> >
> >Also timeout should be unsigned long in that case.
>
> Accepted.
Actually, that's a bad example: what we want to do is to assess success
after we wait, before we decide that something has failed. In other
words, we don't want to wait, and decide that we failed without first
checking for success.
In any case, returning -EFAULT is not sane: EFAULT doesn't mean "fault"
it means "Bad address", and it is returned to userspace to mean that
userspace passed the kernel a bad address. That definition does /not/
fit what's going on here.
timeout = jiffies + msecs_to_jiffies(timeout);
do {
val = dsi_read(dsi, reg);
out = to_set ? (val & status) : !(val & status);
if (out)
break;
if (time_is_after_jiffies(timeout))
return -ETIMEDOUT;
msleep(1);
} while (1);
return 0;
would be better: we only fail immediately after we have checked whether
we succeeded, and we also do the first check immediately.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
[PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver
This patch adds i.MX MIPI DSI host controller driver support.
Currently, the driver supports the burst with sync pulses mode only.
Signed-off-by: Liu Ying
---
.../devicetree/bindings/drm/imx/mipi_dsi.txt | 81 ++
drivers/gpu/drm/imx/Kconfig|6 +
drivers/gpu/drm/imx/Makefile |1 +
drivers/gpu/drm/imx/imx-mipi-dsi.c | 1017
4 files changed, 1105 insertions(+)
create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c
diff --git a/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
new file mode 100644
index 000..3d07fd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt
@@ -0,0 +1,81 @@
+Device-Tree bindings for MIPI DSI host controller
+
+MIPI DSI host controller
+
+
+The MIPI DSI host controller is a Synopsys DesignWare IP.
+It is a digital core that implements all protocol functions defined
+in the MIPI DSI specification, providing an interface between the
+system and the MIPI DPHY, and allowing communication with a MIPI DSI
+compliant display.
+
+Required properties:
+ - #address-cells : Should be <1>.
+ - #size-cells : Should be <0>.
+ - compatible : Should be "fsl,imx6q-mipi-dsi" for i.MX6q/sdl SoCs.
+ - reg : Physical base address of the controller and length of memory
+ mapped region.
+ - interrupts : The controller's interrupt number to the CPU(s).
+ - gpr : Should be <>.
+ The phandle points to the iomuxc-gpr region containing the
+ multiplexer control register for the controller.
+ - clocks, clock-names : Phandles to the controller pllref, pllref_gate
+ and core_cfg clocks, as described in [1] and [2].
+ - panel at 0 : A panel node which contains a display-timings child node as
+ defined in [3].
+ - port@[0-4] : Up to four port nodes with endpoint definitions as defined
+ in [4], corresponding to the four inputs to the controller multiplexer.
+ Note that each port node should contain the input-port property to
+ distinguish it from the panel node, as described in [5].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/imx6q-clock.txt
+[3] Documentation/devicetree/bindings/video/display-timing.txt
+[4] Documentation/devicetree/bindings/media/video-interfaces.txt
+[5] Documentation/devicetree/bindings/mipi/dsi/mipi-dsi-bus.txt
+
+example:
+ gpr: iomuxc-gpr at 020e {
+ /* ... */
+ };
+
+ mipi_dsi: mipi at 021e {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx6q-mipi-dsi";
+ reg = <0x021e 0x4000>;
+ interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>;
+ gpr = <>;
+ clocks = < IMX6QDL_CLK_VIDEO_27M>,
+< IMX6QDL_CLK_HSI_TX>,
+< IMX6QDL_CLK_HSI_TX>;
+ clock-names = "pllref", "pllref_gate", "core_cfg";
+
+ port at 0 {
+ reg = <0>;
+ input-port;
+
+ mipi_mux_0: endpoint {
+ remote-endpoint = <_di0_mipi>;
+ };
+ };
+
+ port at 1 {
+ reg = <1>;
+ input-port;
+
+ mipi_mux_1: endpoint {
+ remote-endpoint = <_di1_mipi>;
+ };
+ };
+
+ panel at 0 {
+ compatible = "himax,hx8369a-dsi";
+ reg = <0>;
+ /* ... */
+
+ display-timings {
+ /* ... */
+ };
+ };
+ };
diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index 82fb758..03f04fb 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -51,3 +51,9 @@ config DRM_IMX_HDMI
depends on DRM_IMX
help
Choose this if you want to use HDMI on i.MX6.
+
+config DRM_IMX_MIPI_DSI
+ tristate "Freescale i.MX DRM MIPI DSI"
+ depends on DRM_IMX && MFD_SYSCON
+ help
+ Choose this if you want to use MIPI DSI on i.MX6.
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index 582c438..4571d52 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
imx-ipuv3-crtc-objs := ipuv3-crtc.o ipuv3-plane.o
obj-$(CONFIG_DRM_IMX_IPUV3)+= imx-ipuv3-crtc.o
obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
+obj-$(CONFIG_DRM_IMX_MIPI_DSI) += imx-mipi-dsi.o
diff --git a/drivers/gpu/drm/imx/imx-mipi-dsi.c
b/drivers/gpu/drm/imx/imx-mipi-dsi.c
new file mode
[PATCH RFC 09/15] drm: imx: Add MIPI DSI host controller driver
On Wed, Dec 10, 2014 at 04:37:22PM +0800, Liu Ying wrote: > This patch adds i.MX MIPI DSI host controller driver support. > Currently, the driver supports the burst with sync pulses mode only. > > Signed-off-by: Liu Ying > --- > .../devicetree/bindings/drm/imx/mipi_dsi.txt | 81 ++ > drivers/gpu/drm/imx/Kconfig|6 + > drivers/gpu/drm/imx/Makefile |1 + > drivers/gpu/drm/imx/imx-mipi-dsi.c | 1017 > > 4 files changed, 1105 insertions(+) > create mode 100644 Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt > create mode 100644 drivers/gpu/drm/imx/imx-mipi-dsi.c > > diff --git a/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt > b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt > new file mode 100644 > index 000..3d07fd7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/drm/imx/mipi_dsi.txt > @@ -0,0 +1,81 @@ > +Device-Tree bindings for MIPI DSI host controller > + > +MIPI DSI host controller > + > + > +The MIPI DSI host controller is a Synopsys DesignWare IP. > +It is a digital core that implements all protocol functions defined > +in the MIPI DSI specification, providing an interface between the > +system and the MIPI DPHY, and allowing communication with a MIPI DSI > +compliant display. > + > +Required properties: > + - #address-cells : Should be <1>. > + - #size-cells : Should be <0>. > + - compatible : Should be "fsl,imx6q-mipi-dsi" for i.MX6q/sdl SoCs. > + - reg : Physical base address of the controller and length of memory > + mapped region. > + - interrupts : The controller's interrupt number to the CPU(s). > + - gpr : Should be <>. > + The phandle points to the iomuxc-gpr region containing the > + multiplexer control register for the controller. Side-note: Shouldn't this really be a pinmux, then? > + - clocks, clock-names : Phandles to the controller pllref, pllref_gate > + and core_cfg clocks, as described in [1] and [2]. > + - panel at 0 : A panel node which contains a display-timings child node as > + defined in [3]. There's no need for these to be named panel@*. They could be bridges for example. And no, they shouldn't contain a display-timings child node either. Panels should have a proper driver and the driver being device specific it should have the timings embedded. > + - port@[0-4] : Up to four port nodes with endpoint definitions as defined > + in [4], corresponding to the four inputs to the controller multiplexer. > + Note that each port node should contain the input-port property to > + distinguish it from the panel node, as described in [5]. [4] says that you can group all port nodes under a ports parent node. I think this is really what you want to do here to make it clear that the ports aren't part of the DSI host binding part of the device. > diff --git a/drivers/gpu/drm/imx/imx-mipi-dsi.c > b/drivers/gpu/drm/imx/imx-mipi-dsi.c [...] > +/* > + * i.MX drm driver - MIPI DSI Host Controller > + * > + * Copyright (C) 2011-2014 Freescale Semiconductor, Inc. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Don't you want the more generic linux/math64.h here? > +#include > +#include > +#include I don't see any of the functions defined in that header used here. > +#include > +#include > +#include > +#include > +#include > + > +#include "imx-drm.h" > + > +#define DRIVER_NAME "imx-mipi-dsi" > + > +#define DSI_VERSION 0x00 > + > +#define DSI_PWR_UP 0x04 > +#define RESET 0 > +#define POWERUP BIT(0) > + > +#define DSI_CLKMGR_CFG 0x08 > +#define TO_CLK_DIVIDSION(div)(((div) & 0xff) << 8) > +#define TX_ESC_CLK_DIVIDSION(div)(((div) & 0xff) << 0) Your indentation here is... weird. > +#define IMX_MIPI_DSI_MAX_DATA_LANES 2 > + > +#define PHY_STATUS_TIMEOUT 10 > +#define CMD_PKT_STATUS_TIMEOUT 20 > + > +#define IMX_MIPI_DSI_PLD_DATA_BUF_SIZE 4 > + > +#define MHZ 100 Why not reuse the existing USEC_PER_SEC? > +#define host_to_dsi(host) container_of(host, struct imx_mipi_dsi, dsi_host) > +#define con_to_dsi(x) container_of(x, struct
