mbox series

[v2,0/4] Update STM DSI PHY driver

Message ID 20231204101113.276368-1-raphael.gallais-pou@foss.st.com
Headers show
Series Update STM DSI PHY driver | expand

Message

Raphael Gallais-Pou Dec. 4, 2023, 10:11 a.m. UTC 
This patch series aims to add several features of the dw-mipi-dsi phy
driver that are missing or need to be updated.

First patch update a PM macro.

Second patch adds runtime PM functionality to the driver.

Third patch adds a clock provider generated by the PHY itself.  As
explained in the commit log of the second patch, a clock declaration is
missing.  Since this clock is parent of 'dsi_k', it leads to an orphan
clock.  Most importantly this patch is an anticipation for future
versions of the DSI PHY, and its inclusion within the display subsystem
and the DRM framework.

Last patch fixes a corner effect introduced previously.  Since 'dsi' and
'dsi_k' are gated by the same bit on the same register, both reference
work as peripheral clock in the device-tree.

---
Changes in v2:
	- Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro
	  and removed __maybe_used for accordingly
	- Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS

Raphael Gallais-Pou (3):
  drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro
  drm/stm: dsi: expose DSI PHY internal clock
  arm: dts: st: fix DSI peripheral clock on stm32mp15 boards

Yannick Fertre (1):
  drm/stm: dsi: add pm runtime ops

 arch/arm/boot/dts/st/stm32mp157.dtsi          |   2 +-
 arch/arm/boot/dts/st/stm32mp157a-dk1-scmi.dts |   2 +-
 arch/arm/boot/dts/st/stm32mp157c-dk2-scmi.dts |   2 +-
 arch/arm/boot/dts/st/stm32mp157c-ed1-scmi.dts |   2 +-
 arch/arm/boot/dts/st/stm32mp157c-ev1-scmi.dts |   2 +-
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c         | 278 +++++++++++++++---
 6 files changed, 242 insertions(+), 46 deletions(-)

Comments

Simon Horman Dec. 8, 2023, 4:58 p.m. UTC | #1 
On Mon, Dec 04, 2023 at 11:11:12AM +0100, Raphael Gallais-Pou wrote:

...

> @@ -514,18 +675,40 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>  		dsi->lane_max_kbps *= 2;
>  	}
>  
> -	dw_mipi_dsi_stm_plat_data.base = dsi->base;
> -	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
> +	dsi->pdata = *pdata;
> +	dsi->pdata.base = dsi->base;
> +	dsi->pdata.priv_data = dsi;
> +
> +	dsi->pdata.max_data_lanes = 2;
> +	dsi->pdata.phy_ops = &dw_mipi_dsi_stm_phy_ops;
>  
>  	platform_set_drvdata(pdev, dsi);
>  
> -	dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> +	dsi->dsi = dw_mipi_dsi_probe(pdev, &dsi->pdata);
>  	if (IS_ERR(dsi->dsi)) {
>  		ret = PTR_ERR(dsi->dsi);
>  		dev_err_probe(dev, ret, "Failed to initialize mipi dsi host\n");
>  		goto err_dsi_probe;
>  	}
>  
> +	/*
> +	 * We need to wait for the generic bridge to probe before enabling and
> +	 * register the internal pixel clock.
> +	 */
> +	ret = clk_prepare_enable(dsi->pclk);
> +	if (ret) {
> +		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
> +		goto err_dsi_probe;
> +	}
> +
> +	ret = dw_mipi_dsi_clk_register(dsi, dev);
> +	if (ret) {
> +		DRM_ERROR("Failed to register DSI pixel clock: %d\n", ret);

Hi Raphael,

Does clk_disable_unprepare(dsi->pclk) need to be added to this unwind
chain?

Flagged by Smatch.

> +		goto err_dsi_probe;
> +	}
> +
> +	clk_disable_unprepare(dsi->pclk);
> +
>  	return 0;
>  
>  err_dsi_probe:

...
Raphael Gallais-Pou Jan. 4, 2024, 1:51 p.m. UTC | #2 
On 12/8/23 17:58, Simon Horman wrote:
> On Mon, Dec 04, 2023 at 11:11:12AM +0100, Raphael Gallais-Pou wrote:
>
> ...
>
>> @@ -514,18 +675,40 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>>  		dsi->lane_max_kbps *= 2;
>>  	}
>>  
>> -	dw_mipi_dsi_stm_plat_data.base = dsi->base;
>> -	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>> +	dsi->pdata = *pdata;
>> +	dsi->pdata.base = dsi->base;
>> +	dsi->pdata.priv_data = dsi;
>> +
>> +	dsi->pdata.max_data_lanes = 2;
>> +	dsi->pdata.phy_ops = &dw_mipi_dsi_stm_phy_ops;
>>  
>>  	platform_set_drvdata(pdev, dsi);
>>  
>> -	dsi->dsi = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
>> +	dsi->dsi = dw_mipi_dsi_probe(pdev, &dsi->pdata);
>>  	if (IS_ERR(dsi->dsi)) {
>>  		ret = PTR_ERR(dsi->dsi);
>>  		dev_err_probe(dev, ret, "Failed to initialize mipi dsi host\n");
>>  		goto err_dsi_probe;
>>  	}
>>  
>> +	/*
>> +	 * We need to wait for the generic bridge to probe before enabling and
>> +	 * register the internal pixel clock.
>> +	 */
>> +	ret = clk_prepare_enable(dsi->pclk);
>> +	if (ret) {
>> +		DRM_ERROR("%s: Failed to enable peripheral clk\n", __func__);
>> +		goto err_dsi_probe;
>> +	}
>> +
>> +	ret = dw_mipi_dsi_clk_register(dsi, dev);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to register DSI pixel clock: %d\n", ret);
> Hi Raphael,

Hi Simon,

You are right,  dsi->clk needs to be disabled in case the clock register fails
before exiting the probe.

I've sent a v3, which normally fixes it.


Regards,

Raphaël

>
> Does clk_disable_unprepare(dsi->pclk) need to be added to this unwind
> chain?
>
> Flagged by Smatch.
>
>> +		goto err_dsi_probe;
>> +	}
>> +
>> +	clk_disable_unprepare(dsi->pclk);
>> +
>>  	return 0;
>>  
>>  err_dsi_probe:
> ...