Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 2/2] drm/rockchip: dw_hdmi: add rk3568 support
To:     Alex Bee <knaerzche@gmail.com>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        hjc@rock-chips.com, heiko@sntech.de, airlied@linux.ie,
        daniel@ffwll.ch, robh+dt@kernel.org, algea.cao@rock-chips.com
Cc:     dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com
References: <20210707120323.401785-1-benjamin.gaignard@collabora.com>
 <20210707120323.401785-3-benjamin.gaignard@collabora.com>
 <a8c5a263-26a6-d4bf-47e7-9266ca1ae5a8@gmail.com>
From:   Andy Yan <andy.yan@rock-chips.com>
Message-ID: <ff1c398c-5140-2cf0-a4aa-b9cf7e0a68d3@rock-chips.com>
Date:   Wed, 14 Jul 2021 09:26:28 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <a8c5a263-26a6-d4bf-47e7-9266ca1ae5a8@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hi Alex:

On 7/13/21 7:40 PM, Alex Bee wrote:
> Hi Benjamin,
>
> Am 07.07.21 um 14:03 schrieb Benjamin Gaignard:
>> Add a new dw_hdmi_plat_data struct and new compatible for rk3568.
>> This version of the HDMI hardware block need two clocks to provide
>> phy reference clock: hclk_vio and hclk.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> version 2:
>> - Add the clocks needed for the phy.
>
> If got Alega's comment correct, it wasn't about the hclks.
> It looks like for this variant, there is another reference clock 
> required (for the phy) like vpll is already (looks like downstream 
> uses HPLL ( = "HDMI-PLL" ?) for that - which also has to switch the 
> frequency according the the drm mode rate - the two clocks you added 
> here are get just enabled (and disabled) here.


Yes, it's HPLL, and the frequency of HPLL and drm mode rate(vop dclk) 
should keep 1:1.

>
> Alega, Andy: Is it really required to enable hclk_vio and hclk(_vop) 
> in the hdmi driver? Are they required to be enabled for the other 
> output variants (i.e. mipi, dsi, rgb ....) as well and shouldn't 
> better be enabled in the (not-yet existing) vop2 driver?


hclk_vop should be enabled, other wise you can't access hdmi registers. 
This is only required for HDMI(mipi dis, edp, rgb don't need it)

>
> Overall: I'm not sure of the benefit of adding this hdmi variant for a 
> SoC where the display driver isn't implemented upstream yet. The 
> "VOP2" IP seems widely new and should probably be ported first. (even 
> if the HDMI part seems a low hanging fruit according to the vendor 
> sources)


Yes, VOP2 IP is widely totaly new and complicated, I have a plan to do 
the upstream. But I am in a rush now, so please give me a lite time????.

>
> Best,
> Alex
>
>>
>>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 68 +++++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c 
>> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> index 830bdd5e9b7ce..dc0e255e45745 100644
>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> @@ -50,6 +50,10 @@
>>   #define RK3399_GRF_SOC_CON20        0x6250
>>   #define RK3399_HDMI_LCDC_SEL        BIT(6)
>>   +#define RK3568_GRF_VO_CON1        0x0364
>> +#define RK3568_HDMI_SDAIN_MSK        BIT(15)
>> +#define RK3568_HDMI_SCLIN_MSK        BIT(14)
>> +
>>   #define HIWORD_UPDATE(val, mask)    (val | (mask) << 16)
>>     /**
>> @@ -71,6 +75,8 @@ struct rockchip_hdmi {
>>       const struct rockchip_hdmi_chip_data *chip_data;
>>       struct clk *vpll_clk;
>>       struct clk *grf_clk;
>> +    struct clk *hclk_vio;
>> +    struct clk *hclk_vop;
>>       struct dw_hdmi *hdmi;
>>       struct phy *phy;
>>   };
>> @@ -216,6 +222,26 @@ static int rockchip_hdmi_parse_dt(struct 
>> rockchip_hdmi *hdmi)
>>           return PTR_ERR(hdmi->grf_clk);
>>       }
>>   +    hdmi->hclk_vio = devm_clk_get(hdmi->dev, "hclk_vio");
>> +    if (PTR_ERR(hdmi->hclk_vio) == -ENOENT) {
>> +        hdmi->hclk_vio = NULL;
>> +    } else if (PTR_ERR(hdmi->hclk_vio) == -EPROBE_DEFER) {
>> +        return -EPROBE_DEFER;
>> +    } else if (IS_ERR(hdmi->hclk_vio)) {
>> +        dev_err(hdmi->dev, "failed to get hclk_vio clock\n");
>> +        return PTR_ERR(hdmi->hclk_vio);
>> +    }
>> +
>> +    hdmi->hclk_vop = devm_clk_get(hdmi->dev, "hclk");
>> +    if (PTR_ERR(hdmi->hclk_vop) == -ENOENT) {
>> +        hdmi->hclk_vop = NULL;
>> +    } else if (PTR_ERR(hdmi->hclk_vop) == -EPROBE_DEFER) {
>> +        return -EPROBE_DEFER;
>> +    } else if (IS_ERR(hdmi->hclk_vop)) {
>> +        dev_err(hdmi->dev, "failed to get hclk_vop clock\n");
>> +        return PTR_ERR(hdmi->hclk_vop);
>> +    }
>> +
>>       return 0;
>>   }
>>   @@ -467,6 +493,19 @@ static const struct dw_hdmi_plat_data 
>> rk3399_hdmi_drv_data = {
>>       .use_drm_infoframe = true,
>>   };
>>   +static struct rockchip_hdmi_chip_data rk3568_chip_data = {
>> +    .lcdsel_grf_reg = -1,
>> +};
>> +
>> +static const struct dw_hdmi_plat_data rk3568_hdmi_drv_data = {
>> +    .mode_valid = dw_hdmi_rockchip_mode_valid,
>> +    .mpll_cfg   = rockchip_mpll_cfg,
>> +    .cur_ctr    = rockchip_cur_ctr,
>> +    .phy_config = rockchip_phy_config,
>> +    .phy_data = &rk3568_chip_data,
>> +    .use_drm_infoframe = true,
>> +};
>> +
>>   static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
>>       { .compatible = "rockchip,rk3228-dw-hdmi",
>>         .data = &rk3228_hdmi_drv_data
>> @@ -480,6 +519,9 @@ static const struct of_device_id 
>> dw_hdmi_rockchip_dt_ids[] = {
>>       { .compatible = "rockchip,rk3399-dw-hdmi",
>>         .data = &rk3399_hdmi_drv_data
>>       },
>> +    { .compatible = "rockchip,rk3568-dw-hdmi",
>> +      .data = &rk3568_hdmi_drv_data
>> +    },
>>       {},
>>   };
>>   MODULE_DEVICE_TABLE(of, dw_hdmi_rockchip_dt_ids);
>> @@ -536,6 +578,28 @@ static int dw_hdmi_rockchip_bind(struct device 
>> *dev, struct device *master,
>>           return ret;
>>       }
>>   +    ret = clk_prepare_enable(hdmi->hclk_vio);
>> +    if (ret) {
>> +        dev_err(hdmi->dev, "Failed to enable HDMI hclk_vio: %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = clk_prepare_enable(hdmi->hclk_vop);
>> +    if (ret) {
>> +        dev_err(hdmi->dev, "Failed to enable HDMI hclk_vop: %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    if (hdmi->chip_data == &rk3568_chip_data) {
>> +        regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
>> +                 HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |
>> +                       RK3568_HDMI_SCLIN_MSK,
>> +                       RK3568_HDMI_SDAIN_MSK |
>> +                       RK3568_HDMI_SCLIN_MSK));
>> +    }
>> +
>>       hdmi->phy = devm_phy_optional_get(dev, "hdmi");
>>       if (IS_ERR(hdmi->phy)) {
>>           ret = PTR_ERR(hdmi->phy);
>> @@ -559,6 +623,8 @@ static int dw_hdmi_rockchip_bind(struct device 
>> *dev, struct device *master,
>>           ret = PTR_ERR(hdmi->hdmi);
>>           drm_encoder_cleanup(encoder);
>>           clk_disable_unprepare(hdmi->vpll_clk);
>> +        clk_disable_unprepare(hdmi->hclk_vio);
>> +        clk_disable_unprepare(hdmi->hclk_vop);
>>       }
>>         return ret;
>> @@ -571,6 +637,8 @@ static void dw_hdmi_rockchip_unbind(struct device 
>> *dev, struct device *master,
>>         dw_hdmi_unbind(hdmi->hdmi);
>>       clk_disable_unprepare(hdmi->vpll_clk);
>> +    clk_disable_unprepare(hdmi->hclk_vio);
>> +    clk_disable_unprepare(hdmi->hclk_vop);
>>   }
>>     static const struct component_ops dw_hdmi_rockchip_ops = {
>>
>
>
>
>