Re: [PATCH v5 19/24] drm/msm/dsi: Add support for SM8750

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 5/5/2025 5:35 AM, Dmitry Baryshkov wrote:
> On Wed, Apr 30, 2025 at 03:00:49PM +0200, Krzysztof Kozlowski wrote:
>> Add support for DSI on Qualcomm SM8750 SoC with notable difference:
>>
>> DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as
>> parents before DSI PHY is configured, the PLLs are prepared and their
>> initial rate is set.  Therefore assigned-clock-parents are not working
>> here and driver is responsible for reparenting clocks with proper
>> procedure: see dsi_clk_init_6g_v2_9().
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Changes in v5:
>> 1. Only reparent byte and pixel clocks while PLLs is prepared. Setting
>>     rate works fine with earlier DISP CC patch for enabling their parents
>>     during rate change.
>>
>> Changes in v3:
>> 1. Drop 'struct msm_dsi_config sm8750_dsi_cfg' and use sm8650 one.
>>
>> SM8750 DSI PHY also needs Dmitry's patch:
>> https://patchwork.freedesktop.org/patch/542000/?series=119177&rev=1
>> (or some other way of correct early setting of the DSI PHY PLL rate)
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 +
>>   drivers/gpu/drm/msm/dsi/dsi_cfg.c  | 14 +++++++
>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  1 +
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 81 ++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 98 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 87496db203d6c7582eadcb74e94eb56a219df292..93c028a122f3a59b1632da76472e0a3e781c6ae8 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -98,6 +98,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi);
>>   int msm_dsi_runtime_suspend(struct device *dev);
>>   int msm_dsi_runtime_resume(struct device *dev);
>>   int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host);
>> +int dsi_link_clk_set_rate_6g_v2_9(struct msm_dsi_host *msm_host);
>>   int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host);
>>   int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
>>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>> @@ -115,6 +116,7 @@ int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *iova);
>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>> +int dsi_clk_init_6g_v2_9(struct msm_dsi_host *msm_host);
>>   int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
>>   int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index 7754dcec33d06e3d6eb8a9d55e53f24af073adb9..7f8a8de0897a579a525b466fd01bbcd95454c614 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -257,6 +257,18 @@ static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {
>>   	.calc_clk_rate = dsi_calc_clk_rate_6g,
>>   };
>>   
>> +static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_9_host_ops = {
>> +	.link_clk_set_rate = dsi_link_clk_set_rate_6g_v2_9,
>> +	.link_clk_enable = dsi_link_clk_enable_6g,
>> +	.link_clk_disable = dsi_link_clk_disable_6g,
>> +	.clk_init_ver = dsi_clk_init_6g_v2_9,
>> +	.tx_buf_alloc = dsi_tx_buf_alloc_6g,
>> +	.tx_buf_get = dsi_tx_buf_get_6g,
>> +	.tx_buf_put = dsi_tx_buf_put_6g,
>> +	.dma_base_get = dsi_dma_base_get_6g,
>> +	.calc_clk_rate = dsi_calc_clk_rate_6g,
>> +};
>> +
>>   static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
>>   	{MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064,
>>   		&apq8064_dsi_cfg, &msm_dsi_v2_host_ops},
>> @@ -300,6 +312,8 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
>>   		&sm8550_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>>   	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_8_0,
>>   		&sm8650_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>> +	{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_9_0,
>> +		&sm8650_dsi_cfg, &msm_dsi_6g_v2_9_host_ops},
>>   };
>>   
>>   const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> index 120cb65164c1ba1deb9acb513e5f073bd560c496..859c279afbb0377d16f8406f3e6b083640aff5a1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> @@ -30,6 +30,7 @@
>>   #define MSM_DSI_6G_VER_MINOR_V2_6_0	0x20060000
>>   #define MSM_DSI_6G_VER_MINOR_V2_7_0	0x20070000
>>   #define MSM_DSI_6G_VER_MINOR_V2_8_0	0x20080000
>> +#define MSM_DSI_6G_VER_MINOR_V2_9_0	0x20090000
>>   
>>   #define MSM_DSI_V2_VER_MINOR_8064	0x0
>>   
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 4d75529c0e858160761f5eb55db65e5d7565c27b..694ed95897d49c477726a2b0bec1099e75a3ce21 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -119,6 +119,15 @@ struct msm_dsi_host {
>>   	struct clk *pixel_clk;
>>   	struct clk *byte_intf_clk;
>>   
>> +	/*
>> +	 * Clocks which needs to be properly parented between DISPCC and DSI PHY
>> +	 * PLL:
>> +	 */
>> +	struct clk *byte_src_clk;
>> +	struct clk *pixel_src_clk;
>> +	struct clk *dsi_pll_byte_clk;
>> +	struct clk *dsi_pll_pixel_clk;
>> +
>>   	unsigned long byte_clk_rate;
>>   	unsigned long byte_intf_clk_rate;
>>   	unsigned long pixel_clk_rate;
>> @@ -269,6 +278,38 @@ int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host)
>>   	return ret;
>>   }
>>   
>> +int dsi_clk_init_6g_v2_9(struct msm_dsi_host *msm_host)
>> +{
>> +	struct device *dev = &msm_host->pdev->dev;
>> +	int ret;
>> +
>> +	ret = dsi_clk_init_6g_v2(msm_host);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msm_host->byte_src_clk = devm_clk_get(dev, "byte_src");
>> +	if (IS_ERR(msm_host->byte_src_clk))
>> +		return dev_err_probe(dev, PTR_ERR(msm_host->byte_src_clk),
>> +				     "can't get byte_src clock\n");
>> +
>> +	msm_host->dsi_pll_byte_clk = devm_clk_get(dev, "dsi_pll_byte");
>> +	if (IS_ERR(msm_host->dsi_pll_byte_clk))
>> +		return dev_err_probe(dev, PTR_ERR(msm_host->dsi_pll_byte_clk),
>> +				     "can't get dsi_pll_byte clock\n");
>> +
>> +	msm_host->pixel_src_clk = devm_clk_get(dev, "pixel_src");
>> +	if (IS_ERR(msm_host->pixel_src_clk))
>> +		return dev_err_probe(dev, PTR_ERR(msm_host->pixel_src_clk),
>> +				     "can't get pixel_src clock\n");
>> +
>> +	msm_host->dsi_pll_pixel_clk = devm_clk_get(dev, "dsi_pll_pixel");
>> +	if (IS_ERR(msm_host->dsi_pll_pixel_clk))
>> +		return dev_err_probe(dev, PTR_ERR(msm_host->dsi_pll_pixel_clk),
>> +				     "can't get dsi_pll_pixel clock\n");
>> +
>> +	return 0;
>> +}
>> +
>>   static int dsi_clk_init(struct msm_dsi_host *msm_host)
>>   {
>>   	struct platform_device *pdev = msm_host->pdev;
>> @@ -370,6 +411,46 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
>>   	return 0;
>>   }
>>   
>> +int dsi_link_clk_set_rate_6g_v2_9(struct msm_dsi_host *msm_host)
>> +{
>> +	struct device *dev = &msm_host->pdev->dev;
>> +	int ret;
>> +
>> +	/*
>> +	 * DSI PHY PLLs have to be enabled to allow reparenting to them and
>> +	 * setting the rates of pixel/byte clocks.
>> +	 */
> 
> According to the docs this should be handled by the
> CLK_OPS_PARENT_ENABLE flag. Please correct me if I'm wrong.
> 

I am also interested to know that if we are indeed setting 
CLK_OPS_PARENT_ENABLE flag, do we need this logic in the dsi driver.

If CLK_OPS_PARENT_ENABLE flag is not working as expected, shouldnt this 
be something fixed on the clk fwk side?

Thanks

Abhinav

>> +	ret = clk_prepare_enable(msm_host->dsi_pll_byte_clk);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable dsi_pll_byte: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(msm_host->dsi_pll_pixel_clk);
> 
> And this.
> 
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable dsi_pll_byte: %d\n", ret);
>> +		goto out_disable_byte_clk;
>> +	}
>> +
>> +	ret = clk_set_parent(msm_host->byte_src_clk, msm_host->dsi_pll_byte_clk);
>> +	if (ret)
>> +		dev_err(dev, "Failed to parent byte_src -> dsi_pll_byte: %d\n", ret);
>> +
>> +	ret = clk_set_parent(msm_host->pixel_src_clk, msm_host->dsi_pll_pixel_clk);
>> +	if (ret)
>> +		dev_err(dev, "Failed to parent pixel_src -> dsi_pll_pixel: %d\n", ret);
>> +
>> +	clk_disable_unprepare(msm_host->dsi_pll_pixel_clk);
>> +	clk_disable_unprepare(msm_host->dsi_pll_byte_clk);
>> +
>> +	return dsi_link_clk_set_rate_6g(msm_host);
>> +
>> +out_disable_byte_clk:
>> +	clk_disable_unprepare(msm_host->dsi_pll_byte_clk);
>> +
>> +	return ret;
>> +}
>> +
>>   int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
>>   {
>>   	int ret;
>>
>> -- 
>> 2.45.2
>>
>