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