Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 10/01/2025 00:18, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote:
>> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two
>> differences worth noting:
>>
>> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their
>>    offsets were just switched.  Currently these registers are not used
>>    in the driver, so the easiest is to document both but keep them
>>    commented out to avoid conflict.
>>
>> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as
>>    parents before they are prepared and 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().
> 
> Isn't it a description of CLK_SET_PARENT_GATE and/or

No - must be gated accross reparent - so opposite.

> CLK_OPS_PARENT_ENABLE ?

Yes, but does not work. Probably enabling parent, before
assigned-clocks-parents, happens still too early:

[    1.623554] DSI PLL(0) lock failed, status=0x00000000
[    1.623556] PLL(0) lock failed
[    1.624650] ------------[ cut here ]------------
[    1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its
configuration.

Or maybe something is missing in the DSI PHY PLL driver?

> 
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi.h                      |  2 +
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.c                  | 25 +++++++
>>  drivers/gpu/drm/msm/dsi/dsi_cfg.h                  |  1 +
>>  drivers/gpu/drm/msm/dsi/dsi_host.c                 | 80 ++++++++++++++++++++++
>>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c              |  2 +
>>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h              |  1 +
>>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c          | 78 +++++++++++++++++++--
>>  .../gpu/drm/msm/registers/display/dsi_phy_7nm.xml  | 14 ++++
> 
> Please separate DSI and PHY changes.
> 
>>  8 files changed, 197 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index 7754dcec33d06e3d6eb8a9d55e53f24af073adb9..e2a8d6fcc45b6c207a3018ea7c8744fcf34dabd2 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -205,6 +205,17 @@ static const struct msm_dsi_config sm8650_dsi_cfg = {
>>  	},
>>  };
>>  
>> +static const struct msm_dsi_config sm8750_dsi_cfg = {
>> +	.io_offset = DSI_6G_REG_SHIFT,
>> +	.regulator_data = sm8650_dsi_regulators,
>> +	.num_regulators = ARRAY_SIZE(sm8650_dsi_regulators),
>> +	.bus_clk_names = dsi_v2_4_clk_names,
>> +	.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>> +	.io_start = {
>> +		{ 0xae94000, 0xae96000 },
>> +	},
>> +};
>> +
>>  static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
>>  	{ .supply = "vdda", .init_load_uA = 8350 },	/* 1.2 V */
>>  	{ .supply = "refgen" },
>> @@ -257,6 +268,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 +323,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,
>> +		&sm8750_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 2218d4f0c5130a0b13f428e89aa30ba2921da572..ced28ee61eedc0a82da9f1d0792f17ee2a5538c4 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");
> 
> How is this going to work in the bonded DSI mode when DSI1 is being fed
> by the DSI0 PLL? For existing platforms this is being handled by
> changing the parents in DT.

I don't see the problem - you just put different clock as input in DTS?

Please trim your replies, so we won't need to keep scrolling to figure
out that there is nothing more to read.

Best regards,
Krzysztof