From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ying.Liu@freescale.com (Liu Ying) Date: Mon, 22 Dec 2014 10:56:54 +0800 Subject: [PATCH RFC v2 08/14] drm: imx: Add MIPI DSI host controller driver In-Reply-To: <1418984246.3165.45.camel@pengutronix.de> References: <1418886696-11636-1-git-send-email-Ying.Liu@freescale.com> <1418886696-11636-9-git-send-email-Ying.Liu@freescale.com> <1418902740.4212.46.camel@pengutronix.de> <5493BD52.8070804@freescale.com> <1418984246.3165.45.camel@pengutronix.de> Message-ID: <54978876.3090408@freescale.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Philipp, On 12/19/2014 06:17 PM, Philipp Zabel wrote: > Hi Liu, > > Am Freitag, den 19.12.2014, 13:53 +0800 schrieb Liu Ying: > [...] >>>> + mipi_dsi: mipi at 021e0000 { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + compatible = "fsl,imx6q-mipi-dsi"; >>>> + reg = <0x021e0000 0x4000>; >>>> + interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>; >>>> + gpr = <&gpr>; >>>> + clocks = <&clks IMX6QDL_CLK_VIDEO_27M>, >>>> + <&clks IMX6QDL_CLK_HSI_TX>, >>>> + <&clks IMX6QDL_CLK_HSI_TX>; >>>> + clock-names = "pllref", "pllref_gate", "core_cfg"; >>> >>> Not sure about this. Are those names from the Synopsys documentation? >> >> No, I don't think it's from there. > > Do you have access to it? I'd like to see the proper names used if > possible, considering this IP core will be used on other SoCs, too. I'm using the Synopsys documentation for Freescale copy. I'm not sure if it may be provided in the open mailing lists. You probably may try [1] to require one copy from Synopsys. [1] https://www.synopsys.com/dw/ipdir.php?ds=mipi_dsi > >>> According to Table 41-1 in the i.MX6Q Reference Manual, this module has >>> 6 clock inputs: >>> - ac_clk_125m (from ahb_clk_root) >>> - pixel_clk (from axi_clk_root) >>> - cfg_clk and pll_refclk (from video_27m) >>> - ips_clk and ipg_clk_s (from ipg_clk_root) >>> The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk", >>> and "pll_refclk" are gated by a single bit called >>> "mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX]. >>> If that is correct, I see no reason for the "pllref_gate" clock. >>> I suppose two clocks "pllref" and "cfg" should suffice. >> >> Using the two clocks makes the code look like this, perhaps: >> >> clocks = <&clks IMX6QDL_CLK_VIDEO_27M>, >> <&clks IMX6QDL_CLK_HSI_TX>; >> clock-names = "pllref", "core_cfg"; >> >> Then, it seems that I have no way to disable the pllref clock if >> using the clock tree after applying this patch set? > > Correct. In my opinion we should put a new gate clock in the clock path > between video_27m and the pllref input triggers the same bit as hsi_tx, > see below. > >> Or, perhaps, this one? >> >> clocks = <&clks IMX6QDL_CLK_HSI_TX>, >> <&clks IMX6QDL_CLK_HSI_TX>; >> clock-names = "pllref", "core_cfg"; >> >> This only uses the gate clock hsi_tx. The current clock tree states >> that it comes from: >> >> pll3_120m -> >> | -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx >> pll2_pfd2_396m -> >> >> So, I can not get the correct pllref clock frequency with this tree. >> The pllref clock should be derived from the video_27m clock. > > According to the i.MX6 reference manual, the cfg clock also is derived > from video_27m, so both have the wrong rate if connected to hsi_tx (yes, > for cfg we don't care about the rate). > > Currently we have this: > > pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx > pll3_pfd1_540m -> video_27m -> hdmi_isfr > > - hsi_tx_podf represents the hsi_tx_clk_root at its output. > - hsi_tx represents the gate between hsi_tx_clk_root and the tx_ref_clk > input to the MIPI_HSI module, which is controlled by the > mipi_core_cfg_clk_enable bit. > - video_27m represents the video_27m_clk_root at its output. > > I'd say we should turn hsi_tx into a shared gate clock and add a new > shared gate clock using the same gate bit. Maybe name it mipi_core_cfg, > after the gating bit in the CCM. > > pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx > pll3_pfd1_540m -> video_27m -> mipi_core_cfg > pll3_pfd1_540m -> video_27m -> hdmi_isfr > > - mipi_core_cfg represents the gate between video_27_clk_root and the > cfg_clk and pllref_clk inputs to the MIPI_DSI module, which is also > controlled by the mipi_core_cfg_clk_enable bit. > >> The way I decided to use the three clocks is: >> 1) PLL related >> * pllref clock only cares about the pll reference rate(the frequency). >> And, the frequency does matter as it has an impact on the lane clock >> frequency. >> * pllref_gate is a gate clock and it only cares about the gate. >> >> 2) register configuration related >> * core_cfg is a gate clock and it only cares about the gate. >> Usually, the register configuration clock frequency is not so important >> and the gate is what we really need. >> >> I am currently not strong on the way I used. I am open to any better >> solution. > > Since cfg is a real clock input to the MIPI DSI IP, that's ok. But the > two pllref entries in reality are one and the same clock input. > >>> Maybe HSI_TX should be split up into multiple shared gate clocks that >>> all set the mipi_core_cfg clock bits (see below). >> >> Yes, maybe. >> If that's the case, do we need to add two gate clocks in the DT node to >> represent cfg_gate and pllref_gate respectively? > > I'd say yes. While on i.MX6 it could be represented by a single clock > because both have the same rate and use the same gate bit, that doesn't > have to be the case on other SoCs. With my suggestion above, that would > be: > > clocks = <&clks IMX6QDL_CLK_MIPI_CORE_CFG>, > <&clks IMX6QDL_CLK_MIPI_CORE_CFG>; > clock-names = "pllref", "cfg"; Your suggestion looks better. I'll implement it in the next version and give you the "Suggested-by" credit. Thanks. > >>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig > [...] >>>> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data) >>>> +{ > [...] >>>> + dsi->pllref_clk = devm_clk_get(dev, "pllref"); >>>> + if (IS_ERR(dsi->pllref_clk)) { >>>> + ret = PTR_ERR(dsi->pllref_clk); >>>> + dev_err(dev, "Unable to get pll reference clock: %d\n", ret); >>>> + return ret; >>>> + } >>>> + clk_prepare_enable(dsi->pllref_clk); > > What I mean below is this: Here you enable pllref ... > > [...] >>>> + dsi->cfg_clk = devm_clk_get(dev, "core_cfg"); >>>> + if (IS_ERR(dsi->cfg_clk)) { >>>> + ret = PTR_ERR(dsi->cfg_clk); >>>> + dev_err(dev, "Unable to get configuration clock: %d\n", ret); >>> >>> And leave pllref enabled? >> >> As I mentioned in the v1-> v2 change log, I enable/disable the >> pllref_clk and pllref_gate_clk at the component binding/unbinding stages >> to help remove the flag 'enabled' introduced in v1. >> >> I referred to the i.MX HDMI driver which enables/disables the isfr clock >> and the iahb clock at the component binding/unbinding stages. >> >> I may try to handle the clock enablement/disablement more decently and >> avoid using the flag 'enable'. >> >>> >>>> + return ret; > > ... and here you return with an error without disabling pllref again. If > the bind fails, unbind won't be called, and the clock stays enabled. For > reference, see how imx-hdmi unprepare_disables its iahb/isfr clocks in > the bind function's error path. I'll improve the logic for the bail-out path. Regards, Liu Ying > >>>> + } >>>> + >>>> + clk_prepare_enable(dsi->cfg_clk); >>>> + val = dsi_read(dsi, DSI_VERSION); >>>> + clk_disable_unprepare(dsi->cfg_clk); >>>> + >>>> + dev_info(dev, "version number is 0x%08x\n", val); >>>> + >>>> + ret = imx_mipi_dsi_register(drm, dsi); >>>> + if (ret) >>> >>> Same here. >> >> You meant that the pllref_gate clock is left enabled above, right? > > Yes. > >> Regards, >> Liu Ying >> >>> >>>> + return ret; > > This return with an error leaves the pllref enabled. > > regards > Philipp > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liu Ying Subject: Re: [PATCH RFC v2 08/14] drm: imx: Add MIPI DSI host controller driver Date: Mon, 22 Dec 2014 10:56:54 +0800 Message-ID: <54978876.3090408@freescale.com> References: <1418886696-11636-1-git-send-email-Ying.Liu@freescale.com> <1418886696-11636-9-git-send-email-Ying.Liu@freescale.com> <1418902740.4212.46.camel@pengutronix.de> <5493BD52.8070804@freescale.com> <1418984246.3165.45.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1418984246.3165.45.camel@pengutronix.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Philipp Zabel Cc: devicetree@vger.kernel.org, linux@arm.linux.org.uk, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, kernel@pengutronix.de, mturquette@linaro.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgUGhpbGlwcCwKCk9uIDEyLzE5LzIwMTQgMDY6MTcgUE0sIFBoaWxpcHAgWmFiZWwgd3JvdGU6 Cj4gSGkgTGl1LAo+Cj4gQW0gRnJlaXRhZywgZGVuIDE5LjEyLjIwMTQsIDEzOjUzICswODAwIHNj aHJpZWIgTGl1IFlpbmc6Cj4gWy4uLl0KPj4+PiArCW1pcGlfZHNpOiBtaXBpQDAyMWUwMDAwIHsK Pj4+PiArCQkjYWRkcmVzcy1jZWxscyA9IDwxPjsKPj4+PiArCQkjc2l6ZS1jZWxscyA9IDwwPjsK Pj4+PiArCQljb21wYXRpYmxlID0gImZzbCxpbXg2cS1taXBpLWRzaSI7Cj4+Pj4gKwkJcmVnID0g PDB4MDIxZTAwMDAgMHg0MDAwPjsKPj4+PiArCQlpbnRlcnJ1cHRzID0gPDAgMTAyIElSUV9UWVBF X0xFVkVMX0hJR0g+Owo+Pj4+ICsJCWdwciA9IDwmZ3ByPjsKPj4+PiArCQljbG9ja3MgPSA8JmNs a3MgSU1YNlFETF9DTEtfVklERU9fMjdNPiwKPj4+PiArCQkJIDwmY2xrcyBJTVg2UURMX0NMS19I U0lfVFg+LAo+Pj4+ICsJCQkgPCZjbGtzIElNWDZRRExfQ0xLX0hTSV9UWD47Cj4+Pj4gKwkJY2xv Y2stbmFtZXMgPSAicGxscmVmIiwgInBsbHJlZl9nYXRlIiwgImNvcmVfY2ZnIjsKPj4+Cj4+PiBO b3Qgc3VyZSBhYm91dCB0aGlzLiBBcmUgdGhvc2UgbmFtZXMgZnJvbSB0aGUgU3lub3BzeXMgZG9j dW1lbnRhdGlvbj8KPj4KPj4gTm8sIEkgZG9uJ3QgdGhpbmsgaXQncyBmcm9tIHRoZXJlLgo+Cj4g RG8geW91IGhhdmUgYWNjZXNzIHRvIGl0PyBJJ2QgbGlrZSB0byBzZWUgdGhlIHByb3BlciBuYW1l cyB1c2VkIGlmCj4gcG9zc2libGUsIGNvbnNpZGVyaW5nIHRoaXMgSVAgY29yZSB3aWxsIGJlIHVz ZWQgb24gb3RoZXIgU29DcywgdG9vLgoKSSdtIHVzaW5nIHRoZSBTeW5vcHN5cyBkb2N1bWVudGF0 aW9uIGZvciBGcmVlc2NhbGUgY29weS4KSSdtIG5vdCBzdXJlIGlmIGl0IG1heSBiZSBwcm92aWRl ZCBpbiB0aGUgb3BlbiBtYWlsaW5nIGxpc3RzLgoKWW91IHByb2JhYmx5IG1heSB0cnkgWzFdIHRv IHJlcXVpcmUgb25lIGNvcHkgZnJvbSBTeW5vcHN5cy4KClsxXSBodHRwczovL3d3dy5zeW5vcHN5 cy5jb20vZHcvaXBkaXIucGhwP2RzPW1pcGlfZHNpCgo+Cj4+PiBBY2NvcmRpbmcgdG8gVGFibGUg NDEtMSBpbiB0aGUgaS5NWDZRIFJlZmVyZW5jZSBNYW51YWwsIHRoaXMgbW9kdWxlIGhhcwo+Pj4g NiBjbG9jayBpbnB1dHM6Cj4+PiAgICAtIGFjX2Nsa18xMjVtIChmcm9tIGFoYl9jbGtfcm9vdCkK Pj4+ICAgIC0gcGl4ZWxfY2xrIChmcm9tIGF4aV9jbGtfcm9vdCkKPj4+ICAgIC0gY2ZnX2NsayBh bmQgcGxsX3JlZmNsayAoZnJvbSB2aWRlb18yN20pCj4+PiAgICAtIGlwc19jbGsgYW5kIGlwZ19j bGtfcyAoZnJvbSBpcGdfY2xrX3Jvb3QpCj4+PiBUaGUgQ0NNIGNoYXB0ZXIgc2F5cyB0aGF0IG9m IHRoZXNlLCAiYWNfY2xrXzEyNW0iLCAiY2ZnX2NsayIsIGlwc19jbGsiLAo+Pj4gYW5kICJwbGxf cmVmY2xrIiBhcmUgZ2F0ZWQgYnkgYSBzaW5nbGUgYml0IGNhbGxlZAo+Pj4gIm1pcGlfY29yZV9j ZmdfY2xrX2VuYWJsZSIsIHRoYXQgaXMgY2xrW0NMS19IU0lfVFhdLgo+Pj4gSWYgdGhhdCBpcyBj b3JyZWN0LCBJIHNlZSBubyByZWFzb24gZm9yIHRoZSAicGxscmVmX2dhdGUiIGNsb2NrLgo+Pj4g SSBzdXBwb3NlIHR3byBjbG9ja3MgInBsbHJlZiIgYW5kICJjZmciIHNob3VsZCBzdWZmaWNlLgo+ Pgo+PiBVc2luZyB0aGUgdHdvIGNsb2NrcyBtYWtlcyB0aGUgY29kZSBsb29rIGxpa2UgdGhpcywg cGVyaGFwczoKPj4KPj4gICAgICAgICBjbG9ja3MgPSA8JmNsa3MgSU1YNlFETF9DTEtfVklERU9f MjdNPiwKPj4gICAgICAgICAgICAgICAgICA8JmNsa3MgSU1YNlFETF9DTEtfSFNJX1RYPjsKPj4g ICAgICAgICBjbG9jay1uYW1lcyA9ICJwbGxyZWYiLCAiY29yZV9jZmciOwo+Pgo+PiBUaGVuLCBp dCBzZWVtcyB0aGF0IEkgaGF2ZSBubyB3YXkgdG8gZGlzYWJsZSB0aGUgcGxscmVmIGNsb2NrIGlm Cj4+IHVzaW5nIHRoZSBjbG9jayB0cmVlIGFmdGVyIGFwcGx5aW5nIHRoaXMgcGF0Y2ggc2V0Pwo+ Cj4gQ29ycmVjdC4gSW4gbXkgb3BpbmlvbiB3ZSBzaG91bGQgcHV0IGEgbmV3IGdhdGUgY2xvY2sg aW4gdGhlIGNsb2NrIHBhdGgKPiBiZXR3ZWVuIHZpZGVvXzI3bSBhbmQgdGhlIHBsbHJlZiBpbnB1 dCB0cmlnZ2VycyB0aGUgc2FtZSBiaXQgYXMgaHNpX3R4LAo+IHNlZSBiZWxvdy4KPgo+PiBPciwg cGVyaGFwcywgdGhpcyBvbmU/Cj4+Cj4+ICAgICAgICAgY2xvY2tzID0gPCZjbGtzIElNWDZRRExf Q0xLX0hTSV9UWD4sCj4+ICAgICAgICAgICAgICAgICAgPCZjbGtzIElNWDZRRExfQ0xLX0hTSV9U WD47Cj4+ICAgICAgICAgY2xvY2stbmFtZXMgPSAicGxscmVmIiwgImNvcmVfY2ZnIjsKPj4KPj4g VGhpcyBvbmx5IHVzZXMgdGhlIGdhdGUgY2xvY2sgaHNpX3R4LiAgVGhlIGN1cnJlbnQgY2xvY2sg dHJlZSBzdGF0ZXMKPj4gdGhhdCBpdCBjb21lcyBmcm9tOgo+Pgo+PiAgICAgICAgcGxsM18xMjBt IC0+Cj4+ICAgICAgICAgICAgICAgICAgICB8IC0+IGhzaV90eF9zZWwgLT4gaHNpX3R4X3BvZGYg LT4gaHNpX3R4Cj4+IHBsbDJfcGZkMl8zOTZtIC0+Cj4+Cj4+IFNvLCBJIGNhbiBub3QgZ2V0IHRo ZSBjb3JyZWN0IHBsbHJlZiBjbG9jayBmcmVxdWVuY3kgd2l0aCB0aGlzIHRyZWUuCj4+IFRoZSBw bGxyZWYgY2xvY2sgc2hvdWxkIGJlIGRlcml2ZWQgZnJvbSB0aGUgdmlkZW9fMjdtIGNsb2NrLgo+ Cj4gQWNjb3JkaW5nIHRvIHRoZSBpLk1YNiByZWZlcmVuY2UgbWFudWFsLCB0aGUgY2ZnIGNsb2Nr IGFsc28gaXMgZGVyaXZlZAo+IGZyb20gdmlkZW9fMjdtLCBzbyBib3RoIGhhdmUgdGhlIHdyb25n IHJhdGUgaWYgY29ubmVjdGVkIHRvIGhzaV90eCAoeWVzLAo+IGZvciBjZmcgd2UgZG9uJ3QgY2Fy ZSBhYm91dCB0aGUgcmF0ZSkuCj4KPiBDdXJyZW50bHkgd2UgaGF2ZSB0aGlzOgo+Cj4gcGxsMl9w ZmQyXzM5Nm0gLT4gaHNpX3R4X3NlbCAtPiBoc2lfdHhfcG9kZiAtPiBoc2lfdHgKPiBwbGwzX3Bm ZDFfNTQwbSAtPiB2aWRlb18yN20gLT4gaGRtaV9pc2ZyCj4KPiAtIGhzaV90eF9wb2RmIHJlcHJl c2VudHMgdGhlIGhzaV90eF9jbGtfcm9vdCBhdCBpdHMgb3V0cHV0Lgo+IC0gaHNpX3R4IHJlcHJl c2VudHMgdGhlIGdhdGUgYmV0d2VlbiBoc2lfdHhfY2xrX3Jvb3QgYW5kIHRoZSB0eF9yZWZfY2xr Cj4gICAgaW5wdXQgdG8gdGhlIE1JUElfSFNJIG1vZHVsZSwgd2hpY2ggaXMgY29udHJvbGxlZCBi eSB0aGUKPiAgICBtaXBpX2NvcmVfY2ZnX2Nsa19lbmFibGUgYml0Lgo+IC0gdmlkZW9fMjdtIHJl cHJlc2VudHMgdGhlIHZpZGVvXzI3bV9jbGtfcm9vdCBhdCBpdHMgb3V0cHV0Lgo+Cj4gSSdkIHNh eSB3ZSBzaG91bGQgdHVybiBoc2lfdHggaW50byBhIHNoYXJlZCBnYXRlIGNsb2NrIGFuZCBhZGQg YSBuZXcKPiBzaGFyZWQgZ2F0ZSBjbG9jayB1c2luZyB0aGUgc2FtZSBnYXRlIGJpdC4gTWF5YmUg bmFtZSBpdCBtaXBpX2NvcmVfY2ZnLAo+IGFmdGVyIHRoZSBnYXRpbmcgYml0IGluIHRoZSBDQ00u Cj4KPiBwbGwyX3BmZDJfMzk2bSAtPiBoc2lfdHhfc2VsIC0+IGhzaV90eF9wb2RmIC0+IGhzaV90 eAo+IHBsbDNfcGZkMV81NDBtIC0+IHZpZGVvXzI3bSAtPiBtaXBpX2NvcmVfY2ZnCj4gcGxsM19w ZmQxXzU0MG0gLT4gdmlkZW9fMjdtIC0+IGhkbWlfaXNmcgo+Cj4gLSBtaXBpX2NvcmVfY2ZnIHJl cHJlc2VudHMgdGhlIGdhdGUgYmV0d2VlbiB2aWRlb18yN19jbGtfcm9vdCBhbmQgdGhlCj4gICAg Y2ZnX2NsayBhbmQgcGxscmVmX2NsayBpbnB1dHMgdG8gdGhlIE1JUElfRFNJIG1vZHVsZSwgd2hp Y2ggaXMgYWxzbwo+ICAgIGNvbnRyb2xsZWQgYnkgdGhlIG1pcGlfY29yZV9jZmdfY2xrX2VuYWJs ZSBiaXQuCj4KPj4gVGhlIHdheSBJIGRlY2lkZWQgdG8gdXNlIHRoZSB0aHJlZSBjbG9ja3MgaXM6 Cj4+IDEpIFBMTCByZWxhdGVkCj4+ICogcGxscmVmIGNsb2NrIG9ubHkgY2FyZXMgYWJvdXQgdGhl IHBsbCByZWZlcmVuY2UgcmF0ZSh0aGUgZnJlcXVlbmN5KS4KPj4gICAgIEFuZCwgdGhlIGZyZXF1 ZW5jeSBkb2VzIG1hdHRlciBhcyBpdCBoYXMgYW4gaW1wYWN0IG9uIHRoZSBsYW5lIGNsb2NrCj4+ ICAgICBmcmVxdWVuY3kuCj4+ICogcGxscmVmX2dhdGUgaXMgYSBnYXRlIGNsb2NrIGFuZCBpdCBv bmx5IGNhcmVzIGFib3V0IHRoZSBnYXRlLgo+Pgo+PiAyKSByZWdpc3RlciBjb25maWd1cmF0aW9u IHJlbGF0ZWQKPj4gKiBjb3JlX2NmZyBpcyBhIGdhdGUgY2xvY2sgYW5kIGl0IG9ubHkgY2FyZXMg YWJvdXQgdGhlIGdhdGUuCj4+IFVzdWFsbHksIHRoZSByZWdpc3RlciBjb25maWd1cmF0aW9uIGNs b2NrIGZyZXF1ZW5jeSBpcyBub3Qgc28gaW1wb3J0YW50Cj4+IGFuZCB0aGUgZ2F0ZSBpcyB3aGF0 IHdlIHJlYWxseSBuZWVkLgo+Pgo+PiBJIGFtIGN1cnJlbnRseSBub3Qgc3Ryb25nIG9uIHRoZSB3 YXkgSSB1c2VkLiAgSSBhbSBvcGVuIHRvIGFueSBiZXR0ZXIKPj4gc29sdXRpb24uCj4KPiBTaW5j ZSBjZmcgaXMgYSByZWFsIGNsb2NrIGlucHV0IHRvIHRoZSBNSVBJIERTSSBJUCwgdGhhdCdzIG9r LiBCdXQgdGhlCj4gdHdvIHBsbHJlZiBlbnRyaWVzIGluIHJlYWxpdHkgYXJlIG9uZSBhbmQgdGhl IHNhbWUgY2xvY2sgaW5wdXQuCj4KPj4+IE1heWJlIEhTSV9UWCBzaG91bGQgYmUgc3BsaXQgdXAg aW50byBtdWx0aXBsZSBzaGFyZWQgZ2F0ZSBjbG9ja3MgdGhhdAo+Pj4gYWxsIHNldCB0aGUgbWlw aV9jb3JlX2NmZyBjbG9jayBiaXRzIChzZWUgYmVsb3cpLgo+Pgo+PiBZZXMsIG1heWJlLgo+PiBJ ZiB0aGF0J3MgdGhlIGNhc2UsIGRvIHdlIG5lZWQgdG8gYWRkIHR3byBnYXRlIGNsb2NrcyBpbiB0 aGUgRFQgbm9kZSB0bwo+PiByZXByZXNlbnQgY2ZnX2dhdGUgYW5kIHBsbHJlZl9nYXRlIHJlc3Bl Y3RpdmVseT8KPgo+IEknZCBzYXkgeWVzLiBXaGlsZSBvbiBpLk1YNiBpdCBjb3VsZCBiZSByZXBy ZXNlbnRlZCBieSBhIHNpbmdsZSBjbG9jawo+IGJlY2F1c2UgYm90aCBoYXZlIHRoZSBzYW1lIHJh dGUgYW5kIHVzZSB0aGUgc2FtZSBnYXRlIGJpdCwgdGhhdCBkb2Vzbid0Cj4gaGF2ZSB0byBiZSB0 aGUgY2FzZSBvbiBvdGhlciBTb0NzLiBXaXRoIG15IHN1Z2dlc3Rpb24gYWJvdmUsIHRoYXQgd291 bGQKPiBiZToKPgo+IAljbG9ja3MgPSA8JmNsa3MgSU1YNlFETF9DTEtfTUlQSV9DT1JFX0NGRz4s Cj4gCQkgPCZjbGtzIElNWDZRRExfQ0xLX01JUElfQ09SRV9DRkc+Owo+IAljbG9jay1uYW1lcyA9 ICJwbGxyZWYiLCAiY2ZnIjsKCllvdXIgc3VnZ2VzdGlvbiBsb29rcyBiZXR0ZXIuIEknbGwgaW1w bGVtZW50IGl0IGluIHRoZSBuZXh0IHZlcnNpb24KYW5kIGdpdmUgeW91IHRoZSAiU3VnZ2VzdGVk LWJ5IiBjcmVkaXQuIFRoYW5rcy4KCj4KPj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJt L2lteC9LY29uZmlnIGIvZHJpdmVycy9ncHUvZHJtL2lteC9LY29uZmlnCj4gWy4uLl0KPj4+PiAr c3RhdGljIGludCBpbXhfbWlwaV9kc2lfYmluZChzdHJ1Y3QgZGV2aWNlICpkZXYsIHN0cnVjdCBk ZXZpY2UgKm1hc3Rlciwgdm9pZCAqZGF0YSkKPj4+PiArewo+IFsuLi5dCj4+Pj4gKwlkc2ktPnBs bHJlZl9jbGsgPSBkZXZtX2Nsa19nZXQoZGV2LCAicGxscmVmIik7Cj4+Pj4gKwlpZiAoSVNfRVJS KGRzaS0+cGxscmVmX2NsaykpIHsKPj4+PiArCQlyZXQgPSBQVFJfRVJSKGRzaS0+cGxscmVmX2Ns ayk7Cj4+Pj4gKwkJZGV2X2VycihkZXYsICJVbmFibGUgdG8gZ2V0IHBsbCByZWZlcmVuY2UgY2xv Y2s6ICVkXG4iLCByZXQpOwo+Pj4+ICsJCXJldHVybiByZXQ7Cj4+Pj4gKwl9Cj4+Pj4gKwljbGtf cHJlcGFyZV9lbmFibGUoZHNpLT5wbGxyZWZfY2xrKTsKPgo+IFdoYXQgSSBtZWFuIGJlbG93IGlz IHRoaXM6IEhlcmUgeW91IGVuYWJsZSBwbGxyZWYgLi4uCj4KPiBbLi4uXQo+Pj4+ICsJZHNpLT5j ZmdfY2xrID0gZGV2bV9jbGtfZ2V0KGRldiwgImNvcmVfY2ZnIik7Cj4+Pj4gKwlpZiAoSVNfRVJS KGRzaS0+Y2ZnX2NsaykpIHsKPj4+PiArCQlyZXQgPSBQVFJfRVJSKGRzaS0+Y2ZnX2Nsayk7Cj4+ Pj4gKwkJZGV2X2VycihkZXYsICJVbmFibGUgdG8gZ2V0IGNvbmZpZ3VyYXRpb24gY2xvY2s6ICVk XG4iLCByZXQpOwo+Pj4KPj4+IEFuZCBsZWF2ZSBwbGxyZWYgZW5hYmxlZD8KPj4KPj4gQXMgSSBt ZW50aW9uZWQgaW4gdGhlIHYxLT4gdjIgY2hhbmdlIGxvZywgSSBlbmFibGUvZGlzYWJsZSB0aGUK Pj4gcGxscmVmX2NsayBhbmQgcGxscmVmX2dhdGVfY2xrIGF0IHRoZSBjb21wb25lbnQgYmluZGlu Zy91bmJpbmRpbmcgc3RhZ2VzCj4+IHRvIGhlbHAgcmVtb3ZlIHRoZSBmbGFnICdlbmFibGVkJyBp bnRyb2R1Y2VkIGluIHYxLgo+Pgo+PiBJIHJlZmVycmVkIHRvIHRoZSBpLk1YIEhETUkgZHJpdmVy IHdoaWNoIGVuYWJsZXMvZGlzYWJsZXMgdGhlIGlzZnIgY2xvY2sKPj4gYW5kIHRoZSBpYWhiIGNs b2NrIGF0IHRoZSBjb21wb25lbnQgYmluZGluZy91bmJpbmRpbmcgc3RhZ2VzLgo+Pgo+PiBJIG1h eSB0cnkgdG8gaGFuZGxlIHRoZSBjbG9jayBlbmFibGVtZW50L2Rpc2FibGVtZW50IG1vcmUgZGVj ZW50bHkgYW5kCj4+IGF2b2lkIHVzaW5nIHRoZSBmbGFnICdlbmFibGUnLgo+Pgo+Pj4KPj4+PiAr CQlyZXR1cm4gcmV0Owo+Cj4gLi4uIGFuZCBoZXJlIHlvdSByZXR1cm4gd2l0aCBhbiBlcnJvciB3 aXRob3V0IGRpc2FibGluZyBwbGxyZWYgYWdhaW4uIElmCj4gdGhlIGJpbmQgZmFpbHMsIHVuYmlu ZCB3b24ndCBiZSBjYWxsZWQsIGFuZCB0aGUgY2xvY2sgc3RheXMgZW5hYmxlZC4gRm9yCj4gcmVm ZXJlbmNlLCBzZWUgaG93IGlteC1oZG1pIHVucHJlcGFyZV9kaXNhYmxlcyBpdHMgaWFoYi9pc2Zy IGNsb2NrcyBpbgo+IHRoZSBiaW5kIGZ1bmN0aW9uJ3MgZXJyb3IgcGF0aC4KCkknbGwgaW1wcm92 ZSB0aGUgbG9naWMgZm9yIHRoZSBiYWlsLW91dCBwYXRoLgoKUmVnYXJkcywKTGl1IFlpbmcKCj4K Pj4+PiArCX0KPj4+PiArCj4+Pj4gKwljbGtfcHJlcGFyZV9lbmFibGUoZHNpLT5jZmdfY2xrKTsK Pj4+PiArCXZhbCA9IGRzaV9yZWFkKGRzaSwgRFNJX1ZFUlNJT04pOwo+Pj4+ICsJY2xrX2Rpc2Fi bGVfdW5wcmVwYXJlKGRzaS0+Y2ZnX2Nsayk7Cj4+Pj4gKwo+Pj4+ICsJZGV2X2luZm8oZGV2LCAi dmVyc2lvbiBudW1iZXIgaXMgMHglMDh4XG4iLCB2YWwpOwo+Pj4+ICsKPj4+PiArCXJldCA9IGlt eF9taXBpX2RzaV9yZWdpc3Rlcihkcm0sIGRzaSk7Cj4+Pj4gKwlpZiAocmV0KQo+Pj4KPj4+IFNh bWUgaGVyZS4KPj4KPj4gWW91IG1lYW50IHRoYXQgdGhlIHBsbHJlZl9nYXRlIGNsb2NrIGlzIGxl ZnQgZW5hYmxlZCBhYm92ZSwgcmlnaHQ/Cj4KPiBZZXMuCj4KPj4gUmVnYXJkcywKPj4gTGl1IFlp bmcKPj4KPj4+Cj4+Pj4gKwkJcmV0dXJuIHJldDsKPgo+IFRoaXMgcmV0dXJuIHdpdGggYW4gZXJy b3IgbGVhdmVzIHRoZSBwbGxyZWYgZW5hYmxlZC4KPgo+IHJlZ2FyZHMKPiBQaGlsaXBwCj4KX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1h aWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753864AbaLVCwk (ORCPT ); Sun, 21 Dec 2014 21:52:40 -0500 Received: from mail-bn1bbn0101.outbound.protection.outlook.com ([157.56.111.101]:12968 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753633AbaLVCwi (ORCPT ); Sun, 21 Dec 2014 21:52:38 -0500 Message-ID: <54978876.3090408@freescale.com> Date: Mon, 22 Dec 2014 10:56:54 +0800 From: Liu Ying User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Philipp Zabel CC: , , , , , , , , , Subject: Re: [PATCH RFC v2 08/14] drm: imx: Add MIPI DSI host controller driver References: <1418886696-11636-1-git-send-email-Ying.Liu@freescale.com> <1418886696-11636-9-git-send-email-Ying.Liu@freescale.com> <1418902740.4212.46.camel@pengutronix.de> <5493BD52.8070804@freescale.com> <1418984246.3165.45.camel@pengutronix.de> In-Reply-To: <1418984246.3165.45.camel@pengutronix.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-EOPAttributedMessage: 0 Authentication-Results: spf=fail (sender IP is 192.88.168.50) smtp.mailfrom=Ying.Liu@freescale.com; X-Forefront-Antispam-Report: CIP:192.88.168.50;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(339900001)(479174004)(199003)(377454003)(189002)(54534003)(24454002)(51704005)(57704003)(83506001)(68736005)(46102003)(19580395003)(21056001)(85426001)(77096005)(2950100001)(23746002)(99396003)(84676001)(120916001)(19580405001)(6806004)(93886004)(97736003)(16799955002)(64126003)(50986999)(54356999)(87266999)(76176999)(65806001)(36756003)(65816999)(64706001)(105606002)(50466002)(86362001)(59896002)(77156002)(47776003)(31966008)(92566001)(15975445007)(87936001)(117636001)(99136001)(107046002)(104016003)(4396001)(62966003)(110136001)(106466001)(20776003)(217873001)(62816006);DIR:OUT;SFP:1102;SCL:1;SRVR:BY2PR0301MB0630;H:tx30smr01.am.freescale.net;FPR:;SPF:Fail;MLV:sfv;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Antispam: UriScan:; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:;SRVR:BY2PR0301MB0630; X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004);SRVR:BY2PR0301MB0630; X-Forefront-PRVS: 0433DB2766 X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:;SRVR:BY2PR0301MB0630; X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Dec 2014 02:52:33.5747 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.168.50] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR0301MB0630 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philipp, On 12/19/2014 06:17 PM, Philipp Zabel wrote: > Hi Liu, > > Am Freitag, den 19.12.2014, 13:53 +0800 schrieb Liu Ying: > [...] >>>> + mipi_dsi: mipi@021e0000 { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + compatible = "fsl,imx6q-mipi-dsi"; >>>> + reg = <0x021e0000 0x4000>; >>>> + interrupts = <0 102 IRQ_TYPE_LEVEL_HIGH>; >>>> + gpr = <&gpr>; >>>> + clocks = <&clks IMX6QDL_CLK_VIDEO_27M>, >>>> + <&clks IMX6QDL_CLK_HSI_TX>, >>>> + <&clks IMX6QDL_CLK_HSI_TX>; >>>> + clock-names = "pllref", "pllref_gate", "core_cfg"; >>> >>> Not sure about this. Are those names from the Synopsys documentation? >> >> No, I don't think it's from there. > > Do you have access to it? I'd like to see the proper names used if > possible, considering this IP core will be used on other SoCs, too. I'm using the Synopsys documentation for Freescale copy. I'm not sure if it may be provided in the open mailing lists. You probably may try [1] to require one copy from Synopsys. [1] https://www.synopsys.com/dw/ipdir.php?ds=mipi_dsi > >>> According to Table 41-1 in the i.MX6Q Reference Manual, this module has >>> 6 clock inputs: >>> - ac_clk_125m (from ahb_clk_root) >>> - pixel_clk (from axi_clk_root) >>> - cfg_clk and pll_refclk (from video_27m) >>> - ips_clk and ipg_clk_s (from ipg_clk_root) >>> The CCM chapter says that of these, "ac_clk_125m", "cfg_clk", ips_clk", >>> and "pll_refclk" are gated by a single bit called >>> "mipi_core_cfg_clk_enable", that is clk[CLK_HSI_TX]. >>> If that is correct, I see no reason for the "pllref_gate" clock. >>> I suppose two clocks "pllref" and "cfg" should suffice. >> >> Using the two clocks makes the code look like this, perhaps: >> >> clocks = <&clks IMX6QDL_CLK_VIDEO_27M>, >> <&clks IMX6QDL_CLK_HSI_TX>; >> clock-names = "pllref", "core_cfg"; >> >> Then, it seems that I have no way to disable the pllref clock if >> using the clock tree after applying this patch set? > > Correct. In my opinion we should put a new gate clock in the clock path > between video_27m and the pllref input triggers the same bit as hsi_tx, > see below. > >> Or, perhaps, this one? >> >> clocks = <&clks IMX6QDL_CLK_HSI_TX>, >> <&clks IMX6QDL_CLK_HSI_TX>; >> clock-names = "pllref", "core_cfg"; >> >> This only uses the gate clock hsi_tx. The current clock tree states >> that it comes from: >> >> pll3_120m -> >> | -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx >> pll2_pfd2_396m -> >> >> So, I can not get the correct pllref clock frequency with this tree. >> The pllref clock should be derived from the video_27m clock. > > According to the i.MX6 reference manual, the cfg clock also is derived > from video_27m, so both have the wrong rate if connected to hsi_tx (yes, > for cfg we don't care about the rate). > > Currently we have this: > > pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx > pll3_pfd1_540m -> video_27m -> hdmi_isfr > > - hsi_tx_podf represents the hsi_tx_clk_root at its output. > - hsi_tx represents the gate between hsi_tx_clk_root and the tx_ref_clk > input to the MIPI_HSI module, which is controlled by the > mipi_core_cfg_clk_enable bit. > - video_27m represents the video_27m_clk_root at its output. > > I'd say we should turn hsi_tx into a shared gate clock and add a new > shared gate clock using the same gate bit. Maybe name it mipi_core_cfg, > after the gating bit in the CCM. > > pll2_pfd2_396m -> hsi_tx_sel -> hsi_tx_podf -> hsi_tx > pll3_pfd1_540m -> video_27m -> mipi_core_cfg > pll3_pfd1_540m -> video_27m -> hdmi_isfr > > - mipi_core_cfg represents the gate between video_27_clk_root and the > cfg_clk and pllref_clk inputs to the MIPI_DSI module, which is also > controlled by the mipi_core_cfg_clk_enable bit. > >> The way I decided to use the three clocks is: >> 1) PLL related >> * pllref clock only cares about the pll reference rate(the frequency). >> And, the frequency does matter as it has an impact on the lane clock >> frequency. >> * pllref_gate is a gate clock and it only cares about the gate. >> >> 2) register configuration related >> * core_cfg is a gate clock and it only cares about the gate. >> Usually, the register configuration clock frequency is not so important >> and the gate is what we really need. >> >> I am currently not strong on the way I used. I am open to any better >> solution. > > Since cfg is a real clock input to the MIPI DSI IP, that's ok. But the > two pllref entries in reality are one and the same clock input. > >>> Maybe HSI_TX should be split up into multiple shared gate clocks that >>> all set the mipi_core_cfg clock bits (see below). >> >> Yes, maybe. >> If that's the case, do we need to add two gate clocks in the DT node to >> represent cfg_gate and pllref_gate respectively? > > I'd say yes. While on i.MX6 it could be represented by a single clock > because both have the same rate and use the same gate bit, that doesn't > have to be the case on other SoCs. With my suggestion above, that would > be: > > clocks = <&clks IMX6QDL_CLK_MIPI_CORE_CFG>, > <&clks IMX6QDL_CLK_MIPI_CORE_CFG>; > clock-names = "pllref", "cfg"; Your suggestion looks better. I'll implement it in the next version and give you the "Suggested-by" credit. Thanks. > >>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig > [...] >>>> +static int imx_mipi_dsi_bind(struct device *dev, struct device *master, void *data) >>>> +{ > [...] >>>> + dsi->pllref_clk = devm_clk_get(dev, "pllref"); >>>> + if (IS_ERR(dsi->pllref_clk)) { >>>> + ret = PTR_ERR(dsi->pllref_clk); >>>> + dev_err(dev, "Unable to get pll reference clock: %d\n", ret); >>>> + return ret; >>>> + } >>>> + clk_prepare_enable(dsi->pllref_clk); > > What I mean below is this: Here you enable pllref ... > > [...] >>>> + dsi->cfg_clk = devm_clk_get(dev, "core_cfg"); >>>> + if (IS_ERR(dsi->cfg_clk)) { >>>> + ret = PTR_ERR(dsi->cfg_clk); >>>> + dev_err(dev, "Unable to get configuration clock: %d\n", ret); >>> >>> And leave pllref enabled? >> >> As I mentioned in the v1-> v2 change log, I enable/disable the >> pllref_clk and pllref_gate_clk at the component binding/unbinding stages >> to help remove the flag 'enabled' introduced in v1. >> >> I referred to the i.MX HDMI driver which enables/disables the isfr clock >> and the iahb clock at the component binding/unbinding stages. >> >> I may try to handle the clock enablement/disablement more decently and >> avoid using the flag 'enable'. >> >>> >>>> + return ret; > > ... and here you return with an error without disabling pllref again. If > the bind fails, unbind won't be called, and the clock stays enabled. For > reference, see how imx-hdmi unprepare_disables its iahb/isfr clocks in > the bind function's error path. I'll improve the logic for the bail-out path. Regards, Liu Ying > >>>> + } >>>> + >>>> + clk_prepare_enable(dsi->cfg_clk); >>>> + val = dsi_read(dsi, DSI_VERSION); >>>> + clk_disable_unprepare(dsi->cfg_clk); >>>> + >>>> + dev_info(dev, "version number is 0x%08x\n", val); >>>> + >>>> + ret = imx_mipi_dsi_register(drm, dsi); >>>> + if (ret) >>> >>> Same here. >> >> You meant that the pllref_gate clock is left enabled above, right? > > Yes. > >> Regards, >> Liu Ying >> >>> >>>> + return ret; > > This return with an error leaves the pllref enabled. > > regards > Philipp >