From mboxrd@z Thu Jan 1 00:00:00 1970 From: architt@codeaurora.org (Archit Taneja) Date: Thu, 16 Apr 2015 11:09:58 +0530 Subject: [PATCH RFC v9 11/20] drm/bridge: Add Synopsys DesignWare MIPI DSI host controller driver In-Reply-To: <20150409084353.GD12103@ulmo> References: <1423720903-24806-1-git-send-email-Ying.Liu@freescale.com> <1423720903-24806-12-git-send-email-Ying.Liu@freescale.com> <20150409084353.GD12103@ulmo> Message-ID: <552F4B2E.2000209@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 04/09/2015 02:13 PM, Thierry Reding wrote: > On Thu, Feb 12, 2015 at 02:01:34PM +0800, Liu Ying wrote: > [...] >> diff --git a/drivers/gpu/drm/bridge/dw_mipi_dsi.c b/drivers/gpu/drm/bridge/dw_mipi_dsi.c > [...] >> +struct dw_mipi_dsi { >> + struct mipi_dsi_host dsi_host; >> + struct drm_connector connector; >> + struct drm_encoder *encoder; >> + struct drm_bridge *bridge; >> + struct drm_panel *panel; >> + struct device *dev; >> + >> + void __iomem *base; >> + >> + struct clk *pllref_clk; >> + struct clk *cfg_clk; >> + struct clk *pclk; >> + >> + unsigned int lane_mbps; /* per lane */ >> + u32 channel; >> + u32 lanes; >> + u32 format; >> + struct drm_display_mode *mode; >> + >> + const struct dw_mipi_dsi_plat_data *pdata; >> + >> + bool enabled; >> +}; > > While reviewing this I kept thinking whether this is really the right > architectural design. This driver is a MIPI DSI host, a connector and > a bridge, all in one. But it seems to me like it should really be an > encoder/connector and a MIPI DSI host. Why the need for a bridge? The > bridge abstraction targets blocks outside of the SoC, but it is my > understanding that these DesignWare IP blocks are designed into SoCs. > The msm driver uses bridges for blocks within the SoC too. We have too many sub blocks in the display controller that use up crtcs and encoder entities. A bridge is the only option one has if an encoder in the display chain is already taken. In the above designware configuration, if some one created a board that used an external chip to further convert DSI to some other output format, then we would be completely exhausted of all entities. I posted a patch that allows us to create a chain of bridges for such cases. It seems to work well as an interim solution. Ideally, it would be better if we could make bridge a special case of an encoder, and let one encoder connect to another encoder. Such a thing would also help us unify i2c slave encoders and bridge drivers too. A chip designed as an i2c slave encoder would work well with a drm driver that doesn't have an encoder, but won't work for SoCs SoCs that already have an encoder and were expecting a bridge entity instead. Archit -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH RFC v9 11/20] drm/bridge: Add Synopsys DesignWare MIPI DSI host controller driver Date: Thu, 16 Apr 2015 11:09:58 +0530 Message-ID: <552F4B2E.2000209@codeaurora.org> References: <1423720903-24806-1-git-send-email-Ying.Liu@freescale.com> <1423720903-24806-12-git-send-email-Ying.Liu@freescale.com> <20150409084353.GD12103@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20150409084353.GD12103@ulmo> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding , Liu Ying Cc: stefan.wahren@i2se.com, devicetree@vger.kernel.org, linux@arm.linux.org.uk, mturquette@linaro.org, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, a.hajda@samsung.com, kernel@pengutronix.de, andy.yan@rock-chips.com, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org CgpPbiAwNC8wOS8yMDE1IDAyOjEzIFBNLCBUaGllcnJ5IFJlZGluZyB3cm90ZToKPiBPbiBUaHUs IEZlYiAxMiwgMjAxNSBhdCAwMjowMTozNFBNICswODAwLCBMaXUgWWluZyB3cm90ZToKPiBbLi4u XQo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kd19taXBpX2RzaS5jIGIv ZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kd19taXBpX2RzaS5jCj4gWy4uLl0KPj4gK3N0cnVjdCBk d19taXBpX2RzaSB7Cj4+ICsJc3RydWN0IG1pcGlfZHNpX2hvc3QgZHNpX2hvc3Q7Cj4+ICsJc3Ry dWN0IGRybV9jb25uZWN0b3IgY29ubmVjdG9yOwo+PiArCXN0cnVjdCBkcm1fZW5jb2RlciAqZW5j b2RlcjsKPj4gKwlzdHJ1Y3QgZHJtX2JyaWRnZSAqYnJpZGdlOwo+PiArCXN0cnVjdCBkcm1fcGFu ZWwgKnBhbmVsOwo+PiArCXN0cnVjdCBkZXZpY2UgKmRldjsKPj4gKwo+PiArCXZvaWQgX19pb21l bSAqYmFzZTsKPj4gKwo+PiArCXN0cnVjdCBjbGsgKnBsbHJlZl9jbGs7Cj4+ICsJc3RydWN0IGNs ayAqY2ZnX2NsazsKPj4gKwlzdHJ1Y3QgY2xrICpwY2xrOwo+PiArCj4+ICsJdW5zaWduZWQgaW50 IGxhbmVfbWJwczsgLyogcGVyIGxhbmUgKi8KPj4gKwl1MzIgY2hhbm5lbDsKPj4gKwl1MzIgbGFu ZXM7Cj4+ICsJdTMyIGZvcm1hdDsKPj4gKwlzdHJ1Y3QgZHJtX2Rpc3BsYXlfbW9kZSAqbW9kZTsK Pj4gKwo+PiArCWNvbnN0IHN0cnVjdCBkd19taXBpX2RzaV9wbGF0X2RhdGEgKnBkYXRhOwo+PiAr Cj4+ICsJYm9vbCBlbmFibGVkOwo+PiArfTsKPgo+IFdoaWxlIHJldmlld2luZyB0aGlzIEkga2Vw dCB0aGlua2luZyB3aGV0aGVyIHRoaXMgaXMgcmVhbGx5IHRoZSByaWdodAo+IGFyY2hpdGVjdHVy YWwgZGVzaWduLiBUaGlzIGRyaXZlciBpcyBhIE1JUEkgRFNJIGhvc3QsIGEgY29ubmVjdG9yIGFu ZAo+IGEgYnJpZGdlLCBhbGwgaW4gb25lLiBCdXQgaXQgc2VlbXMgdG8gbWUgbGlrZSBpdCBzaG91 bGQgcmVhbGx5IGJlIGFuCj4gZW5jb2Rlci9jb25uZWN0b3IgYW5kIGEgTUlQSSBEU0kgaG9zdC4g V2h5IHRoZSBuZWVkIGZvciBhIGJyaWRnZT8gVGhlCj4gYnJpZGdlIGFic3RyYWN0aW9uIHRhcmdl dHMgYmxvY2tzIG91dHNpZGUgb2YgdGhlIFNvQywgYnV0IGl0IGlzIG15Cj4gdW5kZXJzdGFuZGlu ZyB0aGF0IHRoZXNlIERlc2lnbldhcmUgSVAgYmxvY2tzIGFyZSBkZXNpZ25lZCBpbnRvIFNvQ3Mu Cj4KClRoZSBtc20gZHJpdmVyIHVzZXMgYnJpZGdlcyBmb3IgYmxvY2tzIHdpdGhpbiB0aGUgU29D IHRvby4gV2UgaGF2ZSB0b28gCm1hbnkgc3ViIGJsb2NrcyBpbiB0aGUgZGlzcGxheSBjb250cm9s bGVyIHRoYXQgdXNlIHVwIGNydGNzIGFuZCBlbmNvZGVyIAplbnRpdGllcy4gQSBicmlkZ2UgaXMg dGhlIG9ubHkgb3B0aW9uIG9uZSBoYXMgaWYgYW4gZW5jb2RlciBpbiB0aGUgCmRpc3BsYXkgY2hh aW4gaXMgYWxyZWFkeSB0YWtlbi4KCkluIHRoZSBhYm92ZSBkZXNpZ253YXJlIGNvbmZpZ3VyYXRp b24sIGlmIHNvbWUgb25lIGNyZWF0ZWQgYSBib2FyZCB0aGF0IAp1c2VkIGFuIGV4dGVybmFsIGNo aXAgdG8gZnVydGhlciBjb252ZXJ0IERTSSB0byBzb21lIG90aGVyIG91dHB1dCAKZm9ybWF0LCB0 aGVuIHdlIHdvdWxkIGJlIGNvbXBsZXRlbHkgZXhoYXVzdGVkIG9mIGFsbCBlbnRpdGllcy4KCkkg cG9zdGVkIGEgcGF0Y2ggdGhhdCBhbGxvd3MgdXMgdG8gY3JlYXRlIGEgY2hhaW4gb2YgYnJpZGdl cyBmb3Igc3VjaCAKY2FzZXMuIEl0IHNlZW1zIHRvIHdvcmsgd2VsbCBhcyBhbiBpbnRlcmltIHNv bHV0aW9uLiBJZGVhbGx5LCBpdCB3b3VsZCAKYmUgYmV0dGVyIGlmIHdlIGNvdWxkIG1ha2UgYnJp ZGdlIGEgc3BlY2lhbCBjYXNlIG9mIGFuIGVuY29kZXIsIGFuZCBsZXQgCm9uZSBlbmNvZGVyIGNv bm5lY3QgdG8gYW5vdGhlciBlbmNvZGVyLgoKU3VjaCBhIHRoaW5nIHdvdWxkIGFsc28gaGVscCB1 cyB1bmlmeSBpMmMgc2xhdmUgZW5jb2RlcnMgYW5kIGJyaWRnZSAKZHJpdmVycyB0b28uIEEgY2hp cCBkZXNpZ25lZCBhcyBhbiBpMmMgc2xhdmUgZW5jb2RlciB3b3VsZCB3b3JrIHdlbGwgCndpdGgg YSBkcm0gZHJpdmVyIHRoYXQgZG9lc24ndCBoYXZlIGFuIGVuY29kZXIsIGJ1dCB3b24ndCB3b3Jr IGZvciBTb0NzIApTb0NzIHRoYXQgYWxyZWFkeSBoYXZlIGFuIGVuY29kZXIgYW5kIHdlcmUgZXhw ZWN0aW5nIGEgYnJpZGdlIGVudGl0eSAKaW5zdGVhZC4KCkFyY2hpdAoKLS0gClF1YWxjb21tIElu bm92YXRpb24gQ2VudGVyLCBJbmMuIGlzIGEgbWVtYmVyIG9mIENvZGUgQXVyb3JhIEZvcnVtLAph IExpbnV4IEZvdW5kYXRpb24gQ29sbGFib3JhdGl2ZSBQcm9qZWN0Cl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJp LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3Jn L21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752678AbbDPFkL (ORCPT ); Thu, 16 Apr 2015 01:40:11 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:33553 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751012AbbDPFkG (ORCPT ); Thu, 16 Apr 2015 01:40:06 -0400 Message-ID: <552F4B2E.2000209@codeaurora.org> Date: Thu, 16 Apr 2015 11:09:58 +0530 From: Archit Taneja User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Thierry Reding , Liu Ying CC: stefan.wahren@i2se.com, devicetree@vger.kernel.org, linux@arm.linux.org.uk, sboyd@codeaurora.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, a.hajda@samsung.com, kernel@pengutronix.de, andy.yan@rock-chips.com, mturquette@linaro.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH RFC v9 11/20] drm/bridge: Add Synopsys DesignWare MIPI DSI host controller driver References: <1423720903-24806-1-git-send-email-Ying.Liu@freescale.com> <1423720903-24806-12-git-send-email-Ying.Liu@freescale.com> <20150409084353.GD12103@ulmo> In-Reply-To: <20150409084353.GD12103@ulmo> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/09/2015 02:13 PM, Thierry Reding wrote: > On Thu, Feb 12, 2015 at 02:01:34PM +0800, Liu Ying wrote: > [...] >> diff --git a/drivers/gpu/drm/bridge/dw_mipi_dsi.c b/drivers/gpu/drm/bridge/dw_mipi_dsi.c > [...] >> +struct dw_mipi_dsi { >> + struct mipi_dsi_host dsi_host; >> + struct drm_connector connector; >> + struct drm_encoder *encoder; >> + struct drm_bridge *bridge; >> + struct drm_panel *panel; >> + struct device *dev; >> + >> + void __iomem *base; >> + >> + struct clk *pllref_clk; >> + struct clk *cfg_clk; >> + struct clk *pclk; >> + >> + unsigned int lane_mbps; /* per lane */ >> + u32 channel; >> + u32 lanes; >> + u32 format; >> + struct drm_display_mode *mode; >> + >> + const struct dw_mipi_dsi_plat_data *pdata; >> + >> + bool enabled; >> +}; > > While reviewing this I kept thinking whether this is really the right > architectural design. This driver is a MIPI DSI host, a connector and > a bridge, all in one. But it seems to me like it should really be an > encoder/connector and a MIPI DSI host. Why the need for a bridge? The > bridge abstraction targets blocks outside of the SoC, but it is my > understanding that these DesignWare IP blocks are designed into SoCs. > The msm driver uses bridges for blocks within the SoC too. We have too many sub blocks in the display controller that use up crtcs and encoder entities. A bridge is the only option one has if an encoder in the display chain is already taken. In the above designware configuration, if some one created a board that used an external chip to further convert DSI to some other output format, then we would be completely exhausted of all entities. I posted a patch that allows us to create a chain of bridges for such cases. It seems to work well as an interim solution. Ideally, it would be better if we could make bridge a special case of an encoder, and let one encoder connect to another encoder. Such a thing would also help us unify i2c slave encoders and bridge drivers too. A chip designed as an i2c slave encoder would work well with a drm driver that doesn't have an encoder, but won't work for SoCs SoCs that already have an encoder and were expecting a bridge entity instead. Archit -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project