Message-ID: <20150211072128.GA13301@victor>
Date:	Wed, 11 Feb 2015 15:21:46 +0800
From:	Liu Ying <Ying.Liu@...escale.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
CC:	<stefan.wahren@...e.com>, <devicetree@...r.kernel.org>,
	<linux@....linux.org.uk>, <andyshrk@...il.com>,
	<linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
	<a.hajda@...sung.com>, <kernel@...gutronix.de>,
	<mturquette@...aro.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH RFC v8 11/21] Documentation: dt-bindings: Add bindings
 for Synopsys DW MIPI DSI DRM bridge driver

Hi Philipp,

On Fri, Feb 06, 2015 at 04:13:20PM +0800, Liu Ying wrote:
> On Thu, Feb 05, 2015 at 11:10:04AM +0100, Philipp Zabel wrote:
> > Am Mittwoch, den 31.12.2014, 16:23 +0800 schrieb Liu Ying:
> > > This patch adds device tree bindings for Synopsys DesignWare MIPI DSI
> > > host controller DRM bridge driver.
> > > 
> > > Signed-off-by: Liu Ying <Ying.Liu@...escale.com>
> > > ---
> > > v7->v8:
> > >  * None.
> > > 
> > > v6->v7:
> > >  * None.
> > > 
> > > v5->v6:
> > >  * Add the #address-cells and #size-cells properties in the example 'ports'
> > >    node.
> > >  * Remove the useless input-port properties from the example port@0 and port@1
> > >    nodes.
> > > 
> > > v4->v5:
> > >  * None.
> > > 
> > > v3->v4:
> > >  * Newly introduced in v4.  This is separated from the relevant driver patch
> > >    in v3 to address Stefan Wahren's comment.
> > > 
> > >  .../devicetree/bindings/drm/bridge/dw_mipi_dsi.txt | 73 ++++++++++++++++++++++
> > >  1 file changed, 73 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/drm/bridge/dw_mipi_dsi.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_mipi_dsi.txt b/Documentation/devicetree/bindings/drm/bridge/dw_mipi_dsi.txt
> > > new file mode 100644
> > > index 0000000..f88a8d6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/drm/bridge/dw_mipi_dsi.txt
> > > @@ -0,0 +1,73 @@
> > > +Device-Tree bindings for Synopsys DesignWare MIPI DSI host controller
> > > +
> > > +The controller 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: The compatible string should be "fsl,imx6q-mipi-dsi" for
> > > +   i.MX6q/sdl SoCs.  For other SoCs, please refer to their specific
> > > +   device tree binding documentations.
> > 
> > I think the compatible property should additionally contain
> > "snps,dw-mipi-dsi". Also I think other SoCs using the same IP core
> > should eventually list their compatibles here, but that's for later.
> > 
> > How about:
> > + - compatible: The compatible string contain "fsl,imx6q-mipi-dsi" for
> > +   i.MX6q/sdl SoCs.  For other SoCs, please refer to their specific
> > +   device tree binding documentations. A common compatible string
> > +   "snps,dw-mipi-dsi" should be appended for all SoCs.
> 
> I'm not sure if "snps,dw-mipi-dsi" should be appended.
> 
> Is it a compatible string that SoC specific drivers should actually use to
> bind a device?
> 
> As Andy mentioned in [1], DW MIPI DSI embedded in RK618 is configured via I2C
> bus, while i.MX6Q/DL is configured via ARM bus...
> 
> Another concern is that if users only specify "snps,dw-mipi-dsi" in their
> device tree sources and use a kernel which supports multiple platforms,
> say ARM multi v7 platforms, could several enabled SoC specific drivers be
> probed for a single DW MIPI DSI device?
> 
> [1] http://lists.freedesktop.org/archives/dri-devel/2014-December/074344.html
> 
> > 
> > > + - reg: Represent the physical address range of the controller.
> > > + - interrupts: Represent the controller's interrupt to the CPU(s).
> > > + - clocks, clock-names: Phandles to the controller pll reference and
> > > +   core configuration clocks, as described in [1].
> > 
> > From the MIPI CSI-2 datasheets it looks like the D-PHY has a refclk and
> > a cfg_clk input. So I suspect from the name of the "core_cfg" clock,
> > that it actually represents two clock inputs, the "cfg_clk" wired to the
> > D-PHY and a core clock wired to the MIPI DSI host controller. I am not
> > sure if there are designs that control those clocks separately, but I
> > think it'd be safer to split this into two clocks in the device tree.
> 

Our internal MIPI DSI SoC owner gave me some feedbacks on the clock sources.
According to him, the Synopsys DesignWare MIPI DSI host controller needs four
clock sources from an application platform - pclk, refclk, cfg_clk and dpipclk.
These clocks are mentioned in the "DesignWare Cores MIPI DSI Host Controller
Databook, 1.01a1.30a.pdf" documentation.

Quote some words from the documentation:
pclk    - APB clock signal.
refclk  - D-PHY reference clock used for Master-side serial clock generation in
          clock multiplying unit(PLL).
cfg_clk - D-PHY Configuration clock used for the initialization of the PHY. It
          is also used for exiting ULPS state.
dpipclk - Input Pixel clock signal.

The below table reflects how does i.MX6Q/DL provide the pclk, refclk and cfg_clk
for the DesignWare MIPI DSI host controller, according to the SoC owner.
 ----------------------------------------------------------------------------
| Synopsys      |                     i.MX6Q/DL MIPI DSI                     |
| DesignWare    |------------------------------------------------------------|
| documentation |    clock   |     clock root     |       CCM_CCGR bits      |
|---------------|------------|--------------------|--------------------------|
|     pclk      |   ips_clk  |    ipg_clk_root    | mipi_core_cfg_clk_enable |
|---------------|------------|--------------------|--------------------------|
|    refclk     | pll_refclk | video_27m_clk_root | mipi_core_cfg_clk_enable |
|---------------|------------|--------------------|--------------------------|
|    cfg_clk    |   cfg_clk  | video_27m_clk_root | mipi_core_cfg_clk_enable |
 ----------------------------------------------------------------------------

I think we should add a new clock "IMX6QDL_CLK_MIPI_IPG" as a shared clock gate
clock.  And, the clock-names property should exactly contain "pclk", "refclk"
and "cfg_clk", right?

Let me know your thoughts.  Thanks.

> What MIPI CSI-2 datasheets do you refer to?
> I don't find the refclk and cfg_clk in the MIPI CSI chapter of the i.MX6DQ
> Reference Manual v2[2].
> 
> I think we need to know the design philosophy of DW MIPI DSI clock sources
> to settle down the documentation here.  I've asked our internal MIPI DSI
> SoC owner for ideas.  But, someone from Synopsys might be the right
> person, perhaps.
> 
> [2] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf?fasp=1&WT_TYPE=Reference%20Manuals&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation&fileExt=.pdf
> 
> > 
> > Also I am not sure which input to the MIPI DSI host controller the core
> > clock represents. The i.MX6DQ Reference Manual v2 calls the remaining
> > clock inputs gated by mipi_core_cfg_clk_enable "ac_clk_125m" and
> > "ips_clk" (I think the latter is the ABP clock driving the register
> > bank, just called "pclk" in the MIPI CSI-2 documentation).

Yes, the "ips_clk" clock is the "pclk" clock.  The "ac_clk_125m" clock is
designed for test and should not be used by software development, according to
the SoC owner.

Regards,
Liu Ying

> 
> The same MIPI CSI-2 documentation you mentioned above?
> 
> I'm also puzzled about the clocks.
> 
> Regards,
> Liu Ying
> 
> > 
> > regards
> > Philipp
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists