Re: [PATCH v2 4/8] drm: rockchip/dp: add rockchip platform dp driver

Hi Hekio,

在 2015/8/8 6:46, Heiko Stübner 写道:

Hi Yakir,

I think this Rockchip portion is missing a devicetree binding.

Oh, thanks, I would complete it in next ;)

You have the ability to power down the actual edp phy by using grf_edp_iddq_en from GRF_SOC_CON12. This is similar to how the rk3288 usb-phy gets put into a deeper state. So maybe you could provide a phy driver (drivers/phy) for this similar to what the exynos-dp does.

Okay, so I need to add a new phy-rockchip-dp.c, include power_on/power_off ops, thanks.

Some more stuff inline. But I guess by no means complete, as I'm still trying to integrate this into my development-tree.

Am Freitag, 7. August 2015, 05:46:20 schrieb Yakir Yang:

...

Rockchip have three clocks for dp controller, we leave pclk_edp to analogix_dp driver control, and keep the sclk_edp_24m and sclk_edp in platform driver.

Signed-off-by: Yakir Yang ykk@rock-chips.com

Changes in v2: None

drivers/gpu/drm/rockchip/Kconfig | 10 + drivers/gpu/drm/rockchip/Makefile | 1 + drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 419 ++++++++++++++++++++++++ 3 files changed, 430 insertions(+) create mode 100644 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index 35215f6..096ed77 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -25,3 +25,13 @@ config ROCKCHIP_DW_HDMI for the Synopsys DesignWare HDMI driver. If you want to enable HDMI on RK3288 based SoC, you should selet this option.

nit: double blank line

Done,

...

+config ROCKCHIP_ANALOGIX_DP

•    tristate "Rockchip specific extensions for Analogix DP driver"
  

•    depends on DRM_ROCKCHIP
  

•    select DRM_ANALOGIX_DP
  

•    help
  

•  This selects support for Rockchip SoC specific extensions
  

•  for the Analogix Core DP driver. If you want to enable DP
  

•  on RK3288 based SoC, you should selet this option.
  

[...]

...

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c new file mode 100644 index 0000000..2f86e5e --- /dev/null +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -0,0 +1,419 @@ +/*

• • Rockchip SoC DP (Display Port) interface driver.
• • Copyright (C) Fuzhou Rockchip Electronics Co., Ltd.
• • Author: Andy Yan andy.yan@rock-chips.com

• •     Yakir Yang <ykk@rock-chips.com>
    

• •     Jeff Chen <jeff.chen@rock-chips.com>
    

• • 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.
• */

+#include <drm/drmP.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_panel.h> +#include <drm/drm_of.h> +#include <drm/drm_dp_helper.h>

+#include <linux/component.h> +#include <linux/clk.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> +#include <linux/reset.h>

+#include <video/of_videomode.h> +#include <video/videomode.h>

+#include <drm/bridge/analogix_dp.h>

+#include "rockchip_drm_drv.h" +#include "rockchip_drm_vop.h"

+#define encoder_to_dp(c) \

• container_of(c, struct rockchip_dp_device, encoder)
  

+#define plat_data_to_dp(pd) \

• container_of(pd, struct rockchip_dp_device, plat_data)
  

+/* dp grf register offset */ +#define DP_VOP_SEL 0x025c /* grf_soc_con6 */ +#define DP_REF_CLK_SEL 0x0274 /* grf_soc_con12 */

+#define GRF_DP_REF_CLK_SEL_INTER BIT(4) +#define DP_SEL_VOP_LIT BIT(5)

+struct rockchip_dp_device {

• struct drm_device *drm_dev;
• struct device *dev;
• struct drm_encoder encoder;
• struct drm_display_mode mode;
• struct clk *clk_dp;
• struct clk *clk_24m_parent;

this clk_24m_parent does not seem used at all

Done,

...

• struct clk *clk_24m;
• struct regmap *grf;
• struct reset_control *rst;
• struct analogix_dp_plat_data plat_data;

+};

[...]

...

+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 ;)

edp: edp@ff970000 { compatible = "rockchip,rk3288-dp"; reg = <0xff970000 0x4000>; interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;

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>;

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 ?

...

• 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").

...

• 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 :-)

Thanks a lot, - Yakir

[0] https://github.com/mmind/linux-rockchip/blob/devel/somewhat-stable/arch/arm/... [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 <linux/component.h> #include <linux/clk.h> #include <linux/mfd/syscon.h> +#include <linux/of_graph.h> #include <linux/regmap.h> #include <linux/reset.h>

@@ -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< -------------