[v8,08/10] drm: stm: dw-mipi-dsi: let the bridge handle the HW version check
diff mbox series

Message ID 20200427081952.3536741-9-adrian.ratiu@collabora.com
State New
Headers show
Series
  • Genericize DW MIPI DSI bridge and add i.MX 6 driver
Related show

Commit Message

Adrian Ratiu April 27, 2020, 8:19 a.m. UTC
The stm mipi-dsi platform driver added a version test in
commit fa6251a747b7 ("drm/stm: dsi: check hardware version")
so that HW revisions other than v1.3x get rejected. The rockchip
driver had no such check and just assumed register layouts are
v1.3x compatible.

Having such tests was a good idea because only v130/v131 layouts
were supported at the time, however since adding multiple layout
support in the bridge, the version is automatically checked for
all drivers, compatible layouts get picked and unsupported HW is
automatically rejected by the bridge, so there's no use keeping
the test in the stm driver.

The main reason prompting this change is that the stm driver
test immediately disabled the peripheral clock after reading
the version, making the bridge read version 0x0 immediately
after in its own probe(), so we move the clock disabling after
the bridge does the version test.

Tested on STM32F769 and STM32MP1.

Cc: linux-stm32@st-md-mailman.stormreply.com
Reported-by: Adrian Pop <pop.adrian61@gmail.com>
Tested-by: Adrian Pop <pop.adrian61@gmail.com>
Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
New in v6.
---
 drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

Comments

Philippe CORNU May 29, 2020, 3:45 p.m. UTC | #1
Hi Adrian,
and thank you very much for the patchset.
Thank you also for having tested it on STM32F769 and STM32MP1.
Sorry for the late response, Yannick and I will review it as soon as 
possible and we will keep you posted.
Note: Do not hesitate to put us in copy for the next version 
(philippe.cornu@st.com, yannick.fertre@st.com)
Regards,
Philippe :-)


On 4/27/20 10:19 AM, Adrian Ratiu wrote:
> The stm mipi-dsi platform driver added a version test in
> commit fa6251a747b7 ("drm/stm: dsi: check hardware version")
> so that HW revisions other than v1.3x get rejected. The rockchip
> driver had no such check and just assumed register layouts are
> v1.3x compatible.
> 
> Having such tests was a good idea because only v130/v131 layouts
> were supported at the time, however since adding multiple layout
> support in the bridge, the version is automatically checked for
> all drivers, compatible layouts get picked and unsupported HW is
> automatically rejected by the bridge, so there's no use keeping
> the test in the stm driver.
> 
> The main reason prompting this change is that the stm driver
> test immediately disabled the peripheral clock after reading
> the version, making the bridge read version 0x0 immediately
> after in its own probe(), so we move the clock disabling after
> the bridge does the version test.
> 
> Tested on STM32F769 and STM32MP1.
> 
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Reported-by: Adrian Pop <pop.adrian61@gmail.com>
> Tested-by: Adrian Pop <pop.adrian61@gmail.com>
> Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
> New in v6.
> ---
>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> index 2e1f2664495d0..7218e405d7e2b 100644
> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> @@ -402,15 +402,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   		goto err_dsi_probe;
>   	}
>   
> -	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> -	clk_disable_unprepare(pclk);
> -
> -	if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
> -		ret = -ENODEV;
> -		DRM_ERROR("bad dsi hardware version\n");
> -		goto err_dsi_probe;
> -	}
> -
>   	dw_mipi_dsi_stm_plat_data.base = dsi->base;
>   	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>   
> @@ -423,6 +414,9 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>   		goto err_dsi_probe;
>   	}
>   
> +	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
> +	clk_disable_unprepare(pclk);
> +
>   	return 0;
>   
>   err_dsi_probe:
>
Adrian Ratiu June 1, 2020, 9:15 a.m. UTC | #2
On Fri, 29 May 2020, Philippe CORNU <philippe.cornu@st.com> wrote:
> Hi Adrian, and thank you very much for the patchset.  Thank you 
> also for having tested it on STM32F769 and STM32MP1.  Sorry for 
> the late response, Yannick and I will review it as soon as 
> possible and we will keep you posted.  Note: Do not hesitate to 
> put us in copy for the next version  (philippe.cornu@st.com, 
> yannick.fertre@st.com) Regards, Philippe :-) 

Hi Philippe,

Thank you very much for your previous and future STM testing, 
really appreciate it! I've CC'd Yannick until now but I'll also CC 
you sure :)

It's been over a month since I posted v8 and I was just gearing up 
to address all feedback, rebase & retest to prepare v9 but I'll 
wait a little longer, no problem, it's no rush.

Have an awesome day,
Adrian

>
>
> On 4/27/20 10:19 AM, Adrian Ratiu wrote:
>> The stm mipi-dsi platform driver added a version test in
>> commit fa6251a747b7 ("drm/stm: dsi: check hardware version")
>> so that HW revisions other than v1.3x get rejected. The rockchip
>> driver had no such check and just assumed register layouts are
>> v1.3x compatible.
>> 
>> Having such tests was a good idea because only v130/v131 layouts
>> were supported at the time, however since adding multiple layout
>> support in the bridge, the version is automatically checked for
>> all drivers, compatible layouts get picked and unsupported HW is
>> automatically rejected by the bridge, so there's no use keeping
>> the test in the stm driver.
>> 
>> The main reason prompting this change is that the stm driver
>> test immediately disabled the peripheral clock after reading
>> the version, making the bridge read version 0x0 immediately
>> after in its own probe(), so we move the clock disabling after
>> the bridge does the version test.
>> 
>> Tested on STM32F769 and STM32MP1.
>> 
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Reported-by: Adrian Pop <pop.adrian61@gmail.com>
>> Tested-by: Adrian Pop <pop.adrian61@gmail.com>
>> Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>> New in v6.
>> ---
>>   drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> index 2e1f2664495d0..7218e405d7e2b 100644
>> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> @@ -402,15 +402,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>>   		goto err_dsi_probe;
>>   	}
>>   
>> -	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
>> -	clk_disable_unprepare(pclk);
>> -
>> -	if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
>> -		ret = -ENODEV;
>> -		DRM_ERROR("bad dsi hardware version\n");
>> -		goto err_dsi_probe;
>> -	}
>> -
>>   	dw_mipi_dsi_stm_plat_data.base = dsi->base;
>>   	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>>   
>> @@ -423,6 +414,9 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>>   		goto err_dsi_probe;
>>   	}
>>   
>> +	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
>> +	clk_disable_unprepare(pclk);
>> +
>>   	return 0;
>>   
>>   err_dsi_probe:
>>
Emil Velikov June 2, 2020, 12:53 p.m. UTC | #3
Hi Adrian,

On Mon, 1 Jun 2020 at 10:14, Adrian Ratiu <adrian.ratiu@collabora.com> wrote:
>
> On Fri, 29 May 2020, Philippe CORNU <philippe.cornu@st.com> wrote:
> > Hi Adrian, and thank you very much for the patchset.  Thank you
> > also for having tested it on STM32F769 and STM32MP1.  Sorry for
> > the late response, Yannick and I will review it as soon as
> > possible and we will keep you posted.  Note: Do not hesitate to
> > put us in copy for the next version  (philippe.cornu@st.com,
> > yannick.fertre@st.com) Regards, Philippe :-)
>
> Hi Philippe,
>
> Thank you very much for your previous and future STM testing,
> really appreciate it! I've CC'd Yannick until now but I'll also CC
> you sure :)
>
> It's been over a month since I posted v8 and I was just gearing up
> to address all feedback, rebase & retest to prepare v9 but I'll
> wait a little longer, no problem, it's no rush.
>
Small idea, pardon for joining so late:

Might be a good idea to add inline comment, why the clocks are disabled so late.
Effectively a 2 line version of the commit summary.

Feel free to make that a separate/follow-up patch.

-Emil
Adrian Ratiu June 3, 2020, 10:28 a.m. UTC | #4
On Tue, 02 Jun 2020, Emil Velikov <emil.l.velikov@gmail.com> 
wrote:
> Hi Adrian, 

Hi Email,

> 
> On Mon, 1 Jun 2020 at 10:14, Adrian Ratiu 
> <adrian.ratiu@collabora.com> wrote: 
>> 
>> On Fri, 29 May 2020, Philippe CORNU <philippe.cornu@st.com> 
>> wrote: 
>> > Hi Adrian, and thank you very much for the patchset.  Thank 
>> > you also for having tested it on STM32F769 and STM32MP1. 
>> > Sorry for the late response, Yannick and I will review it as 
>> > soon as possible and we will keep you posted.  Note: Do not 
>> > hesitate to put us in copy for the next version 
>> > (philippe.cornu@st.com, yannick.fertre@st.com) Regards, 
>> > Philippe :-) 
>> 
>> Hi Philippe, 
>> 
>> Thank you very much for your previous and future STM testing, 
>> really appreciate it! I've CC'd Yannick until now but I'll also 
>> CC you sure :) 
>> 
>> It's been over a month since I posted v8 and I was just gearing 
>> up to address all feedback, rebase & retest to prepare v9 but 
>> I'll wait a little longer, no problem, it's no rush. 
>> 
> Small idea, pardon for joining so late: 
> 
> Might be a good idea to add inline comment, why the clocks are 
> disabled so late.  Effectively a 2 line version of the commit 
> summary. 
> 
> Feel free to make that a separate/follow-up patch.

Thanks, I'll add the comment to this patch in v9.

>
> -Emil
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Patch
diff mbox series

diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
index 2e1f2664495d0..7218e405d7e2b 100644
--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
@@ -402,15 +402,6 @@  static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 		goto err_dsi_probe;
 	}
 
-	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
-	clk_disable_unprepare(pclk);
-
-	if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
-		ret = -ENODEV;
-		DRM_ERROR("bad dsi hardware version\n");
-		goto err_dsi_probe;
-	}
-
 	dw_mipi_dsi_stm_plat_data.base = dsi->base;
 	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
 
@@ -423,6 +414,9 @@  static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
 		goto err_dsi_probe;
 	}
 
+	dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
+	clk_disable_unprepare(pclk);
+
 	return 0;
 
 err_dsi_probe: