Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753372AbbHJNR2 (ORCPT ); Mon, 10 Aug 2015 09:17:28 -0400 Received: from regular1.263xmail.com ([211.150.99.135]:38452 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751470AbbHJNRZ (ORCPT ); Mon, 10 Aug 2015 09:17:25 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: nel@lists.infradead.org X-SENDER-IP: 172.245.164.6 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: <2a35c5526b333353dc049c389a95b4c9> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver To: =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1438943674-18191-1-git-send-email-ykk@rock-chips.com> <1730542.s7otqjtiXD@diego> <55C57D7E.6080800@rock-chips.com> <2657934.h7jy1M3E7a@diego> Cc: Russell King , Fabio Estevam , Jingoo Han , Inki Dae , djkurtz@google.com, dianders@google.com, seanpaul@google.com, joe@perches.com, Takashi Iwai , Andrzej Hajda , Thierry Reding , Philipp Zabel , David Airlie , Gustavo Padovan , Seung-Woo Kim , Kyungmin Park , Krzysztof Kozlowski , Kukjin Kim , Ajay Kumar , Joonyoung Shim , Vincent Palatin , Mark Yao , Andy Yan , ajaynumb@gmail.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-ker@null.null, nel@lists.infradead.org From: Yakir Yang Message-ID: <55C8A3FA.6040100@rock-chips.com> Date: Mon, 10 Aug 2015 21:15:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <2657934.h7jy1M3E7a@diego> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8466 Lines: 253 Hi Heiko, 在 2015/8/10 20:08, Heiko Stübner 写道: > Hi Yakir, > > Am Samstag, 8. August 2015, 11:54:38 schrieb Yakir Yang: >>>> +static int rockchip_dp_init(struct rockchip_dp_device *dp) >>>> +{ >>>> + struct device *dev = dp->dev; >>>> + struct device_node *np = dev->of_node; >>>> + int ret; >>>> + >>>> + dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf"); >>>> + if (IS_ERR(dp->grf)) { >>>> + dev_err(dev, >>>> + "rk3288-dp needs rockchip,grf property\n"); >>>> + return PTR_ERR(dp->grf); >>>> + } >>>> + >>>> + dp->clk_dp = devm_clk_get(dev, "clk_dp"); >>> I've looked at the manual, but couldn't find an actual clock-name >>> used there. Is it really "clk_dp" or should it just be "dp"? >> This should be "clk_dp", not "dp". >> Cause analogix_dp_core would need a clock name with "dp", so I would >> rather to pasted my rockchip-dp node here before I add dt-bindings in >> next version ;) > The clock we name PCLK_EDP_CTRL in the clock controller is probably the clock > supplying the APB interface and named pclk already in the "Figure 3-2 > DP_TXclock domain" diagram on page 19 of the manual. So your "clk_dp" should > actually be "pclk". > > So you would have "dp", "dp_24m" and "pclk" for the 3 supplying clocks. Oh, yes, "pclk" is for APB interface, and "sclk_edp" for IP controller, and "sclk_edp_24m" for DP PHY, thanks for your explain. So for now, I would pass "sclk_edp" to "edp" in analogix_dp, and "sclk_edp_24m" to "dp-phy_24m" in phy-rockchip-dp.c, and "pclk_edp" to "pclk" in analogix_dp-rockchip.c. > >> edp: edp@ff970000 { >> compatible = "rockchip,rk3288-dp"; >> reg = <0xff970000 0x4000>; >> interrupts = ; >> >> clocks = <&cru SCLK_EDP>, <&cru SCLK_EDP_24M>, <&cru >> PCLK_EDP_CTRL>; >> clock-names = "clk_dp", "clk_dp_24m", "dp"; >> >> rockchip,grf = <&grf>; >> resets = <&cru 111>; >> reset-names = "dp"; >> power-domains = <&power RK3288_PD_VIO>; >> status = "disabled"; >> >> hsync-active-high = <0>; >> vsync-active-high = <0>; >> interlaced = <0>; >> samsung,color-space = <0>; >> samsung,dynamic-range = <0>; >> samsung,ycbcr-coeff = <0>; >> samsung,color-depth = <1>; >> samsung,link-rate = <0x0a>; >> samsung,lane-count = <1>; > Thierry already said, that these should probably be somehow auto-detected. > Properties needing to stay around should probably also be "analogix,..." with > a fallback to not break Samsung devicetrees, so > look for "analogix,foo!, if not found try "samsung,foo" Okay, it's better to rename to "analogxi...", done. > >> ports { >> edp_in: port { >> #address-cells = <1>; >> #size-cells = <0>; >> edp_in_vopb: endpoint@0 { >> reg = <0>; >> remote-endpoint = <&vopb_out_edp>; >> }; >> }; >> }; > > >>>> + >>>> + dp->clk_24m = devm_clk_get(dev, "clk_dp_24m"); >>> Same here, maybe "dp_24m". >> Like my previous reply. And actually as those two clocks all have >> a common prefix "SCLK" in rk3288 clock tree, I thinkt we can name >> them to "sclk_dp" & "sclk_dp_24m", is it okay ? > As Thierry said, please don't add prefixes. Okay, so is it okay to rename them to "dp", "dp-phy-24m", "pclk" ? > >>>> + if (IS_ERR(dp->clk_24m)) { >>>> + dev_err(dev, "cannot get clk_dp_24m\n"); >>>> + return PTR_ERR(dp->clk_24m); >>>> + } >>> I think you're missing the pclk here (PCLK_EDP_CTRL) or is this part of >>> something else? >> Whops, as I refered in commit message I leave pclk_dp to >> analogix_dp_core driver ;-) >> >> The reason why I want to leave pclk is I thought this clock is more like >> analogix dp >> core driver want, like a IP controller clock (whatever analogix_dp do >> need a clock >> named with "dp"). > Hmm, I'd think what the core (and Samsung) driver use as "dp" clock is > probably the generic clock for the IP and not the pclk for the APB interface. > > So I think it still should be "dp" for the core and "dp_24m" + "pclk" for the > rockchip part? Yes, I think you are right, thanks ;) > >>>> + >>>> + dp->rst = devm_reset_control_get(dev, "dp"); >>>> + if (IS_ERR(dp->rst)) { >>>> + dev_err(dev, "failed to get reset\n"); >>>> + return PTR_ERR(dp->rst); >>>> + } >>>> + >>>> + ret = rockchip_dp_clk_enable(dp); >>>> + if (ret < 0) { >>>> + dev_err(dp->dev, "cannot enable dp clk %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = rockchip_dp_pre_init(dp); >>>> + if (ret < 0) { >>>> + dev_err(dp->dev, "failed to pre init %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>> [...] >>> >>>> +static int rockchip_dp_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct device_node *panel_node; >>>> + struct rockchip_dp_device *dp; >>>> + struct drm_panel *panel; >>>> + >>>> + panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); >>>> + if (!panel_node) { >>>> + DRM_ERROR("failed to find rockchip,panel dt node\n"); >>>> + return -ENODEV; >>>> + } >>> Personally I would prefer to continue with the of-graph framework to >>> attach the panel instead of defining a special node. But I'm not >>> authorative on this. But that way the dts could then look like [0]. >>> >>> I've sucessfully modified the driver currently in use in Chromeos for my >>> dev-tree and have ported this change over onto your driver [1]. >> Wow! looks very nice, and really appricate for your ported code ;) >> >> BTW should I rebase on your patch, or can just take your code in my next >> version :-) > The code I currently carry, is from the ChromeOS tree, so of course nothing in > mainline should be based on it. You could simply include the change into your > driver. Okay, Thanks, - Yakir > > Heiko > >> >> Thanks a lot, >> - Yakir >> >>> [0] >>> https://github.com/mmind/linux-rockchip/blob/devel/somewhat-stable/arch/a >>> rm/boot/dts/rk3288-veyron-edp.dtsi#L76 [1] >>> ---------- 8< ------------- >>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index e7cf9ab..24e872d >>> 100644 >>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> @@ -20,6 +20,7 @@ >>> >>> #include >>> #include >>> #include >>> >>> +#include >>> >>> #include >>> #include >>> >>> @@ -335,14 +336,28 @@ static const struct component_ops >>> rockchip_dp_component_ops = {> >>> static int rockchip_dp_probe(struct platform_device *pdev) >>> { >>> >>> struct device *dev = &pdev->dev; >>> >>> - struct device_node *panel_node; >>> + struct device_node *panel_node, *port, *endpoint; >>> >>> struct rockchip_dp_device *dp; >>> struct drm_panel *panel; >>> >>> - panel_node = of_parse_phandle(dev->of_node, "rockchip,panel", 0); >>> + port = of_graph_get_port_by_id(dev->of_node, 1); >>> + if (!port) { >>> + dev_err(dev, "can't find output port\n"); >>> + return -EINVAL; >>> + } >>> + >>> + endpoint = of_get_child_by_name(port, "endpoint"); >>> + of_node_put(port); >>> + if (!endpoint) { >>> + dev_err(dev, "no output endpoint found\n"); >>> + return -EINVAL; >>> + } >>> + >>> + panel_node = of_graph_get_remote_port_parent(endpoint); >>> + of_node_put(endpoint); >>> >>> if (!panel_node) { >>> >>> - DRM_ERROR("failed to find rockchip,panel dt node\n"); >>> - return -ENODEV; >>> + dev_err(&pdev->dev, "no output node found\n"); >>> + return -EINVAL; >>> >>> } >>> >>> panel = of_drm_find_panel(panel_node); >>> >>> ---------- 8< ------------- > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/