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

[Yakir Yang <ykk@rock-chips.com>]

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