[PATCH v3,0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence

[PATCH v3,0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In upstream-v5.8, dsi_enable will operate panel_enable, but this
modification has been moved in v5.9. In order to ensure the timing of
dsi_power_on/off and the timing of pulling up/down the MIPI signal,
the modification of v5.9 is synchronized in this series of patches.

Changes since v2:
1. Rebase linux-next

Changes since v1:
1. Dsi sequence marked with patch adjustment
2. Fixes: mtk_dsi: Use the drm_panel_bridge

Jitao Shi (3):
  drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11
  drm/mediatek: Separate poweron/poweroff from enable/disable and define
    new funcs
  drm/mediatek: keep dsi as LP00 before dcs cmds transfer

Xinlei Lee (1):
  drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff
    function

 drivers/gpu/drm/mediatek/mtk_dsi.c | 74 ++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 24 deletions(-)

-- 
2.18.0

[PATCH v3, 0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In upstream-v5.8, dsi_enable will operate panel_enable, but this
modification has been moved in v5.9. In order to ensure the timing of
dsi_power_on/off and the timing of pulling up/down the MIPI signal,
the modification of v5.9 is synchronized in this series of patches.

Changes since v2:
1. Rebase linux-next

Changes since v1:
1. Dsi sequence marked with patch adjustment
2. Fixes: mtk_dsi: Use the drm_panel_bridge

Jitao Shi (3):
  drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11
  drm/mediatek: Separate poweron/poweroff from enable/disable and define
    new funcs
  drm/mediatek: keep dsi as LP00 before dcs cmds transfer

Xinlei Lee (1):
  drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff
    function

 drivers/gpu/drm/mediatek/mtk_dsi.c | 74 ++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 24 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3, 0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In upstream-v5.8, dsi_enable will operate panel_enable, but this
modification has been moved in v5.9. In order to ensure the timing of
dsi_power_on/off and the timing of pulling up/down the MIPI signal,
the modification of v5.9 is synchronized in this series of patches.

Changes since v2:
1. Rebase linux-next

Changes since v1:
1. Dsi sequence marked with patch adjustment
2. Fixes: mtk_dsi: Use the drm_panel_bridge

Jitao Shi (3):
  drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11
  drm/mediatek: Separate poweron/poweroff from enable/disable and define
    new funcs
  drm/mediatek: keep dsi as LP00 before dcs cmds transfer

Xinlei Lee (1):
  drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff
    function

 drivers/gpu/drm/mediatek/mtk_dsi.c | 74 ++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 24 deletions(-)

-- 
2.18.0


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

[PATCH v3, 0/4] Cooperate with DSI RX devices to modify dsi funcs and delay mipi high to cooperate with panel sequence

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In upstream-v5.8, dsi_enable will operate panel_enable, but this
modification has been moved in v5.9. In order to ensure the timing of
dsi_power_on/off and the timing of pulling up/down the MIPI signal,
the modification of v5.9 is synchronized in this series of patches.

Changes since v2:
1. Rebase linux-next

Changes since v1:
1. Dsi sequence marked with patch adjustment
2. Fixes: mtk_dsi: Use the drm_panel_bridge

Jitao Shi (3):
  drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11
  drm/mediatek: Separate poweron/poweroff from enable/disable and define
    new funcs
  drm/mediatek: keep dsi as LP00 before dcs cmds transfer

Xinlei Lee (1):
  drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff
    function

 drivers/gpu/drm/mediatek/mtk_dsi.c | 74 ++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 24 deletions(-)

-- 
2.18.0

[PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

Old sequence:
1. Pull the MIPI signal high
2. Delay & Dsi_reset
3. Set the dsi timing register
4. dsi clk & lanes leave ulp mode and enter hs mode

New sequence:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later patch.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index ccb0511b9cd5..262c027d8c2f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_phy_timconfig(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_ps_control_vact(dsi);
 	mtk_dsi_set_vm_cmd(dsi);
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
+	mtk_dsi_rxtx_control(dsi);
+	usleep_range(30, 100);
+	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_clk_ulp_mode_leave(dsi);
 	mtk_dsi_lane0_ulp_mode_leave(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 0);
-- 
2.18.0

[PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

Old sequence:
1. Pull the MIPI signal high
2. Delay & Dsi_reset
3. Set the dsi timing register
4. dsi clk & lanes leave ulp mode and enter hs mode

New sequence:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later patch.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index ccb0511b9cd5..262c027d8c2f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_phy_timconfig(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_ps_control_vact(dsi);
 	mtk_dsi_set_vm_cmd(dsi);
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
+	mtk_dsi_rxtx_control(dsi);
+	usleep_range(30, 100);
+	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_clk_ulp_mode_leave(dsi);
 	mtk_dsi_lane0_ulp_mode_leave(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 0);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

Old sequence:
1. Pull the MIPI signal high
2. Delay & Dsi_reset
3. Set the dsi timing register
4. dsi clk & lanes leave ulp mode and enter hs mode

New sequence:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later patch.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index ccb0511b9cd5..262c027d8c2f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_phy_timconfig(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_ps_control_vact(dsi);
 	mtk_dsi_set_vm_cmd(dsi);
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
+	mtk_dsi_rxtx_control(dsi);
+	usleep_range(30, 100);
+	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_clk_ulp_mode_leave(dsi);
 	mtk_dsi_lane0_ulp_mode_leave(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 0);
-- 
2.18.0


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

[PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

Old sequence:
1. Pull the MIPI signal high
2. Delay & Dsi_reset
3. Set the dsi timing register
4. dsi clk & lanes leave ulp mode and enter hs mode

New sequence:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later patch.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index ccb0511b9cd5..262c027d8c2f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_phy_timconfig(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_ps_control_vact(dsi);
 	mtk_dsi_set_vm_cmd(dsi);
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
+	mtk_dsi_rxtx_control(dsi);
+	usleep_range(30, 100);
+	mtk_dsi_reset_dphy(dsi);
 	mtk_dsi_clk_ulp_mode_leave(dsi);
 	mtk_dsi_lane0_ulp_mode_leave(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 0);
-- 
2.18.0

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[Rex-BC Chen]

Hello Xinlei,

Thanks for your patch, and there are something I want to know:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 

Could you explain why you want to change original power on sequence?
From this commit message, I don't know why you want to do this.
If the reason is in cover letter, I think you should move them here.

> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think this is no need to describe here.

BRs,
Rex
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[Rex-BC Chen]

Hello Xinlei,

Thanks for your patch, and there are something I want to know:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 

Could you explain why you want to change original power on sequence?
From this commit message, I don't know why you want to do this.
If the reason is in cover letter, I think you should move them here.

> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think this is no need to describe here.

BRs,
Rex
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[Rex-BC Chen]

Hello Xinlei,

Thanks for your patch, and there are something I want to know:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 

Could you explain why you want to change original power on sequence?
From this commit message, I don't know why you want to do this.
If the reason is in cover letter, I think you should move them here.

> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think this is no need to describe here.

BRs,
Rex
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[Rex-BC Chen]

Hello Xinlei,

Thanks for your patch, and there are something I want to know:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 

Could you explain why you want to change original power on sequence?
From this commit message, I don't know why you want to do this.
If the reason is in cover letter, I think you should move them here.

> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think this is no need to describe here.

BRs,
Rex
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);


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

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Thu, 2022-03-17 at 19:13 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> Thanks for your patch, and there are something I want to know:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> 
> Could you explain why you want to change original power on sequence?
> From this commit message, I don't know why you want to do this.
> If the reason is in cover letter, I think you should move them here.
> 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think this is no need to describe here.
> 
> BRs,
> Rex
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 
Hi Rex:

Thanks for your review!
Explain to you that this change does not actually affect the function
of dsi. We have verified this.
After this modification, it is convenient to move them into the if
(!dsi->lanes_ready) condition, which is just an adjustment action.
If you still have questions about my later explanation, please give
another suggestion.

Thanks!
xinlei

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Thu, 2022-03-17 at 19:13 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> Thanks for your patch, and there are something I want to know:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> 
> Could you explain why you want to change original power on sequence?
> From this commit message, I don't know why you want to do this.
> If the reason is in cover letter, I think you should move them here.
> 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think this is no need to describe here.
> 
> BRs,
> Rex
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 
Hi Rex:

Thanks for your review!
Explain to you that this change does not actually affect the function
of dsi. We have verified this.
After this modification, it is convenient to move them into the if
(!dsi->lanes_ready) condition, which is just an adjustment action.
If you still have questions about my later explanation, please give
another suggestion.

Thanks!
xinlei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Thu, 2022-03-17 at 19:13 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> Thanks for your patch, and there are something I want to know:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> 
> Could you explain why you want to change original power on sequence?
> From this commit message, I don't know why you want to do this.
> If the reason is in cover letter, I think you should move them here.
> 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think this is no need to describe here.
> 
> BRs,
> Rex
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 
Hi Rex:

Thanks for your review!
Explain to you that this change does not actually affect the function
of dsi. We have verified this.
After this modification, it is convenient to move them into the if
(!dsi->lanes_ready) condition, which is just an adjustment action.
If you still have questions about my later explanation, please give
another suggestion.

Thanks!
xinlei

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

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[CK Hu]

Hi, Xinlei:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think there would be one patch in 5.9 make the wrong sequence, so add
'Fixes' tag to indicate which patch make the wrong sequence. Use the
term correct/wrong instead old/new sequence.

I still do not understand what is the sequence after apply this patch?

Does the sequence is this after apply this patch?
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

Regards,
CK

> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[CK Hu]

Hi, Xinlei:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think there would be one patch in 5.9 make the wrong sequence, so add
'Fixes' tag to indicate which patch make the wrong sequence. Use the
term correct/wrong instead old/new sequence.

I still do not understand what is the sequence after apply this patch?

Does the sequence is this after apply this patch?
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

Regards,
CK

> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[CK Hu]

Hi, Xinlei:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think there would be one patch in 5.9 make the wrong sequence, so add
'Fixes' tag to indicate which patch make the wrong sequence. Use the
term correct/wrong instead old/new sequence.

I still do not understand what is the sequence after apply this patch?

Does the sequence is this after apply this patch?
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

Regards,
CK

> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[CK Hu]

Hi, Xinlei:

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> Old sequence:
> 1. Pull the MIPI signal high
> 2. Delay & Dsi_reset
> 3. Set the dsi timing register
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> New sequence:
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> patch.

I think there would be one patch in 5.9 make the wrong sequence, so add
'Fixes' tag to indicate which patch make the wrong sequence. Use the
term correct/wrong instead old/new sequence.

I still do not understand what is the sequence after apply this patch?

Does the sequence is this after apply this patch?
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

Regards,
CK

> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index ccb0511b9cd5..262c027d8c2f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_ps_control_vact(dsi);
>  	mtk_dsi_set_vm_cmd(dsi);
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> +	mtk_dsi_rxtx_control(dsi);
> +	usleep_range(30, 100);
> +	mtk_dsi_reset_dphy(dsi);
>  	mtk_dsi_clk_ulp_mode_leave(dsi);
>  	mtk_dsi_lane0_ulp_mode_leave(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 0);


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

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Mon, 2022-03-21 at 17:36 +0800, CK Hu wrote:
> Hi, Xinlei:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think there would be one patch in 5.9 make the wrong sequence, so
> add
> 'Fixes' tag to indicate which patch make the wrong sequence. Use the
> term correct/wrong instead old/new sequence.
> 
> I still do not understand what is the sequence after apply this
> patch?
> 
> Does the sequence is this after apply this patch?
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> Regards,
> CK
> 
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 

Hi CK:

Thanks for your review!

You are right, the sequence after patching is:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

This modification will not affect the dsi function, just to put the
operation of pulling up the mipi signal in poweron together to
facilitate the separation from the poweron function later.

I will add the "Fixes" tag here as well.

BR!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Mon, 2022-03-21 at 17:36 +0800, CK Hu wrote:
> Hi, Xinlei:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think there would be one patch in 5.9 make the wrong sequence, so
> add
> 'Fixes' tag to indicate which patch make the wrong sequence. Use the
> term correct/wrong instead old/new sequence.
> 
> I still do not understand what is the sequence after apply this
> patch?
> 
> Does the sequence is this after apply this patch?
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> Regards,
> CK
> 
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 

Hi CK:

Thanks for your review!

You are right, the sequence after patching is:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

This modification will not affect the dsi function, just to put the
operation of pulling up the mipi signal in poweron together to
facilitate the separation from the poweron function later.

I will add the "Fixes" tag here as well.

BR!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3, 1/4] drm/mediatek: Adjust the timing of mipi signal from LP00 to LP11

[xinlei.lee]

On Mon, 2022-03-21 at 17:36 +0800, CK Hu wrote:
> Hi, Xinlei:
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > Old sequence:
> > 1. Pull the MIPI signal high
> > 2. Delay & Dsi_reset
> > 3. Set the dsi timing register
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > New sequence:
> > 1. Set the dsi timing register
> > 2. Pull the MIPI signal high
> > 3. Delay & Dsi_reset
> > 4. dsi clk & lanes leave ulp mode and enter hs mode
> > 
> > In the new sequence 2 & 3 & 4 will be moved to dsi_enbale in later
> > patch.
> 
> I think there would be one patch in 5.9 make the wrong sequence, so
> add
> 'Fixes' tag to indicate which patch make the wrong sequence. Use the
> term correct/wrong instead old/new sequence.
> 
> I still do not understand what is the sequence after apply this
> patch?
> 
> Does the sequence is this after apply this patch?
> 1. Set the dsi timing register
> 2. Pull the MIPI signal high
> 3. Delay & Dsi_reset
> 4. dsi clk & lanes leave ulp mode and enter hs mode
> 
> Regards,
> CK
> 
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index ccb0511b9cd5..262c027d8c2f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -649,14 +649,14 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_phy_timconfig(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_ps_control_vact(dsi);
> >  	mtk_dsi_set_vm_cmd(dsi);
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > +	mtk_dsi_rxtx_control(dsi);
> > +	usleep_range(30, 100);
> > +	mtk_dsi_reset_dphy(dsi);
> >  	mtk_dsi_clk_ulp_mode_leave(dsi);
> >  	mtk_dsi_lane0_ulp_mode_leave(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 0);
> 
> 

Hi CK:

Thanks for your review!

You are right, the sequence after patching is:
1. Set the dsi timing register
2. Pull the MIPI signal high
3. Delay & Dsi_reset
4. dsi clk & lanes leave ulp mode and enter hs mode

This modification will not affect the dsi function, just to put the
operation of pulling up the mipi signal in poweron together to
facilitate the separation from the poweron function later.

I will add the "Fixes" tag here as well.

BR!
xinlei

[PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

In order to match the changes of "Use the drm_panel_bridge API",
the poweron/poweroff of dsi is extracted from enable/disable and
defined as new funcs (pre_enable/post_disable).

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 262c027d8c2f..e33caaca11a7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	if (--dsi->refcount != 0)
 		return;
 
-	/*
-	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
-	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
-	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
-	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
-	 * after dsi is fully set.
-	 */
-	mtk_dsi_stop(dsi);
-
-	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 {
-	int ret;
-
 	if (dsi->enabled)
 		return;
 
-	ret = mtk_dsi_poweron(dsi);
-	if (ret < 0) {
-		DRM_ERROR("failed to power on dsi\n");
-		return;
-	}
-
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
 	if (!dsi->enabled)
 		return;
 
-	mtk_dsi_poweroff(dsi);
+	/*
+	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
+	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
+	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
+	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
+	 * after dsi is fully set.
+	 */
+	mtk_dsi_stop(dsi);
+
+	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 
 	dsi->enabled = false;
 }
@@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge)
 	mtk_output_dsi_enable(dsi);
 }
 
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweron(dsi);
+}
+
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweroff(dsi);
+}
+
 static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
 	.attach = mtk_dsi_bridge_attach,
 	.disable = mtk_dsi_bridge_disable,
 	.enable = mtk_dsi_bridge_enable,
+	.pre_enable = mtk_dsi_bridge_pre_enable,
+	.post_disable = mtk_dsi_bridge_post_disable,
 	.mode_set = mtk_dsi_bridge_mode_set,
 };
 
-- 
2.18.0

[PATCH v3, 2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

In order to match the changes of "Use the drm_panel_bridge API",
the poweron/poweroff of dsi is extracted from enable/disable and
defined as new funcs (pre_enable/post_disable).

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 262c027d8c2f..e33caaca11a7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	if (--dsi->refcount != 0)
 		return;
 
-	/*
-	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
-	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
-	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
-	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
-	 * after dsi is fully set.
-	 */
-	mtk_dsi_stop(dsi);
-
-	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 {
-	int ret;
-
 	if (dsi->enabled)
 		return;
 
-	ret = mtk_dsi_poweron(dsi);
-	if (ret < 0) {
-		DRM_ERROR("failed to power on dsi\n");
-		return;
-	}
-
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
 	if (!dsi->enabled)
 		return;
 
-	mtk_dsi_poweroff(dsi);
+	/*
+	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
+	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
+	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
+	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
+	 * after dsi is fully set.
+	 */
+	mtk_dsi_stop(dsi);
+
+	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 
 	dsi->enabled = false;
 }
@@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge)
 	mtk_output_dsi_enable(dsi);
 }
 
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweron(dsi);
+}
+
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweroff(dsi);
+}
+
 static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
 	.attach = mtk_dsi_bridge_attach,
 	.disable = mtk_dsi_bridge_disable,
 	.enable = mtk_dsi_bridge_enable,
+	.pre_enable = mtk_dsi_bridge_pre_enable,
+	.post_disable = mtk_dsi_bridge_post_disable,
 	.mode_set = mtk_dsi_bridge_mode_set,
 };
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3, 2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

In order to match the changes of "Use the drm_panel_bridge API",
the poweron/poweroff of dsi is extracted from enable/disable and
defined as new funcs (pre_enable/post_disable).

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 262c027d8c2f..e33caaca11a7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	if (--dsi->refcount != 0)
 		return;
 
-	/*
-	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
-	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
-	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
-	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
-	 * after dsi is fully set.
-	 */
-	mtk_dsi_stop(dsi);
-
-	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 {
-	int ret;
-
 	if (dsi->enabled)
 		return;
 
-	ret = mtk_dsi_poweron(dsi);
-	if (ret < 0) {
-		DRM_ERROR("failed to power on dsi\n");
-		return;
-	}
-
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
 	if (!dsi->enabled)
 		return;
 
-	mtk_dsi_poweroff(dsi);
+	/*
+	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
+	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
+	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
+	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
+	 * after dsi is fully set.
+	 */
+	mtk_dsi_stop(dsi);
+
+	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 
 	dsi->enabled = false;
 }
@@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge)
 	mtk_output_dsi_enable(dsi);
 }
 
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweron(dsi);
+}
+
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweroff(dsi);
+}
+
 static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
 	.attach = mtk_dsi_bridge_attach,
 	.disable = mtk_dsi_bridge_disable,
 	.enable = mtk_dsi_bridge_enable,
+	.pre_enable = mtk_dsi_bridge_pre_enable,
+	.post_disable = mtk_dsi_bridge_post_disable,
 	.mode_set = mtk_dsi_bridge_mode_set,
 };
 
-- 
2.18.0


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

[PATCH v3, 2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

In order to match the changes of "Use the drm_panel_bridge API",
the poweron/poweroff of dsi is extracted from enable/disable and
defined as new funcs (pre_enable/post_disable).

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 262c027d8c2f..e33caaca11a7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	if (--dsi->refcount != 0)
 		return;
 
-	/*
-	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
-	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
-	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
-	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
-	 * after dsi is fully set.
-	 */
-	mtk_dsi_stop(dsi);
-
-	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 {
-	int ret;
-
 	if (dsi->enabled)
 		return;
 
-	ret = mtk_dsi_poweron(dsi);
-	if (ret < 0) {
-		DRM_ERROR("failed to power on dsi\n");
-		return;
-	}
-
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
 	if (!dsi->enabled)
 		return;
 
-	mtk_dsi_poweroff(dsi);
+	/*
+	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
+	 * mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(),
+	 * which needs irq for vblank, and mtk_dsi_stop() will disable irq.
+	 * mtk_dsi_start() needs to be called in mtk_output_dsi_enable(),
+	 * after dsi is fully set.
+	 */
+	mtk_dsi_stop(dsi);
+
+	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 
 	dsi->enabled = false;
 }
@@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge)
 	mtk_output_dsi_enable(dsi);
 }
 
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweron(dsi);
+}
+
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
+
+	mtk_dsi_poweroff(dsi);
+}
+
 static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
 	.attach = mtk_dsi_bridge_attach,
 	.disable = mtk_dsi_bridge_disable,
 	.enable = mtk_dsi_bridge_enable,
+	.pre_enable = mtk_dsi_bridge_pre_enable,
+	.post_disable = mtk_dsi_bridge_post_disable,
 	.mode_set = mtk_dsi_bridge_mode_set,
 };
 
-- 
2.18.0

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> In order to match the changes of "Use the drm_panel_bridge API",
> the poweron/poweroff of dsi is extracted from enable/disable and
> defined as new funcs (pre_enable/post_disable).
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----------
> --
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 262c027d8c2f..e33caaca11a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  	if (--dsi->refcount != 0)
>  		return;
>  
> -	/*
> -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> -	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> -	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> -	 * after dsi is fully set.
> -	 */
> -	mtk_dsi_stop(dsi);
> -
> -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
>  {
> -	int ret;
> -
>  	if (dsi->enabled)
>  		return;
>  
> -	ret = mtk_dsi_poweron(dsi);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to power on dsi\n");
> -		return;
> -	}
> -
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> mtk_dsi *dsi)
>  	if (!dsi->enabled)
>  		return;
>  
> -	mtk_dsi_poweroff(dsi);
> +	/*
> +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since

Why they are asymmetric?

> +	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> +	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> +	 * after dsi is fully set.
> +	 */
> +	mtk_dsi_stop(dsi);
> +
> +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  
>  	dsi->enabled = false;
>  }
> @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	mtk_output_dsi_enable(dsi);
>  }
>  
> +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweron(dsi);

Should you handle the error of mtk_dsi_poweron?
If you failed to mtk_dsi_bridge_pre_enable and do
mtk_dsi_bridge_enable,
what will happend?

> +}
> +
> +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweroff(dsi);

If you failed to mtk_dsi_bridge_disable and you do
mtk_dsi_bridge_post_disable,
what will happend?
Do you need to handle this?

BRs,
Rex

> +}
> +
>  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
>  	.attach = mtk_dsi_bridge_attach,
>  	.disable = mtk_dsi_bridge_disable,
>  	.enable = mtk_dsi_bridge_enable,
> +	.pre_enable = mtk_dsi_bridge_pre_enable,
> +	.post_disable = mtk_dsi_bridge_post_disable,
>  	.mode_set = mtk_dsi_bridge_mode_set,
>  };
>  

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> In order to match the changes of "Use the drm_panel_bridge API",
> the poweron/poweroff of dsi is extracted from enable/disable and
> defined as new funcs (pre_enable/post_disable).
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----------
> --
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 262c027d8c2f..e33caaca11a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  	if (--dsi->refcount != 0)
>  		return;
>  
> -	/*
> -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> -	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> -	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> -	 * after dsi is fully set.
> -	 */
> -	mtk_dsi_stop(dsi);
> -
> -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
>  {
> -	int ret;
> -
>  	if (dsi->enabled)
>  		return;
>  
> -	ret = mtk_dsi_poweron(dsi);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to power on dsi\n");
> -		return;
> -	}
> -
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> mtk_dsi *dsi)
>  	if (!dsi->enabled)
>  		return;
>  
> -	mtk_dsi_poweroff(dsi);
> +	/*
> +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since

Why they are asymmetric?

> +	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> +	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> +	 * after dsi is fully set.
> +	 */
> +	mtk_dsi_stop(dsi);
> +
> +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  
>  	dsi->enabled = false;
>  }
> @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	mtk_output_dsi_enable(dsi);
>  }
>  
> +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweron(dsi);

Should you handle the error of mtk_dsi_poweron?
If you failed to mtk_dsi_bridge_pre_enable and do
mtk_dsi_bridge_enable,
what will happend?

> +}
> +
> +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweroff(dsi);

If you failed to mtk_dsi_bridge_disable and you do
mtk_dsi_bridge_post_disable,
what will happend?
Do you need to handle this?

BRs,
Rex

> +}
> +
>  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
>  	.attach = mtk_dsi_bridge_attach,
>  	.disable = mtk_dsi_bridge_disable,
>  	.enable = mtk_dsi_bridge_enable,
> +	.pre_enable = mtk_dsi_bridge_pre_enable,
> +	.post_disable = mtk_dsi_bridge_post_disable,
>  	.mode_set = mtk_dsi_bridge_mode_set,
>  };
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> In order to match the changes of "Use the drm_panel_bridge API",
> the poweron/poweroff of dsi is extracted from enable/disable and
> defined as new funcs (pre_enable/post_disable).
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----------
> --
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 262c027d8c2f..e33caaca11a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  	if (--dsi->refcount != 0)
>  		return;
>  
> -	/*
> -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> -	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> -	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> -	 * after dsi is fully set.
> -	 */
> -	mtk_dsi_stop(dsi);
> -
> -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
>  {
> -	int ret;
> -
>  	if (dsi->enabled)
>  		return;
>  
> -	ret = mtk_dsi_poweron(dsi);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to power on dsi\n");
> -		return;
> -	}
> -
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> mtk_dsi *dsi)
>  	if (!dsi->enabled)
>  		return;
>  
> -	mtk_dsi_poweroff(dsi);
> +	/*
> +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since

Why they are asymmetric?

> +	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> +	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> +	 * after dsi is fully set.
> +	 */
> +	mtk_dsi_stop(dsi);
> +
> +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  
>  	dsi->enabled = false;
>  }
> @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	mtk_output_dsi_enable(dsi);
>  }
>  
> +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweron(dsi);

Should you handle the error of mtk_dsi_poweron?
If you failed to mtk_dsi_bridge_pre_enable and do
mtk_dsi_bridge_enable,
what will happend?

> +}
> +
> +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweroff(dsi);

If you failed to mtk_dsi_bridge_disable and you do
mtk_dsi_bridge_post_disable,
what will happend?
Do you need to handle this?

BRs,
Rex

> +}
> +
>  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
>  	.attach = mtk_dsi_bridge_attach,
>  	.disable = mtk_dsi_bridge_disable,
>  	.enable = mtk_dsi_bridge_enable,
> +	.pre_enable = mtk_dsi_bridge_pre_enable,
> +	.post_disable = mtk_dsi_bridge_post_disable,
>  	.mode_set = mtk_dsi_bridge_mode_set,
>  };
>  


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

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> In order to match the changes of "Use the drm_panel_bridge API",
> the poweron/poweroff of dsi is extracted from enable/disable and
> defined as new funcs (pre_enable/post_disable).
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----------
> --
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 262c027d8c2f..e33caaca11a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  	if (--dsi->refcount != 0)
>  		return;
>  
> -	/*
> -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> -	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> -	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> -	 * after dsi is fully set.
> -	 */
> -	mtk_dsi_stop(dsi);
> -
> -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> *dsi)
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
>  {
> -	int ret;
> -
>  	if (dsi->enabled)
>  		return;
>  
> -	ret = mtk_dsi_poweron(dsi);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to power on dsi\n");
> -		return;
> -	}
> -
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> mtk_dsi *dsi)
>  	if (!dsi->enabled)
>  		return;
>  
> -	mtk_dsi_poweroff(dsi);
> +	/*
> +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since

Why they are asymmetric?

> +	 * mtk_dsi_stop() should be called after
> mtk_drm_crtc_atomic_disable(),
> +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> irq.
> +	 * mtk_dsi_start() needs to be called in
> mtk_output_dsi_enable(),
> +	 * after dsi is fully set.
> +	 */
> +	mtk_dsi_stop(dsi);
> +
> +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>  
>  	dsi->enabled = false;
>  }
> @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	mtk_output_dsi_enable(dsi);
>  }
>  
> +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweron(dsi);

Should you handle the error of mtk_dsi_poweron?
If you failed to mtk_dsi_bridge_pre_enable and do
mtk_dsi_bridge_enable,
what will happend?

> +}
> +
> +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	mtk_dsi_poweroff(dsi);

If you failed to mtk_dsi_bridge_disable and you do
mtk_dsi_bridge_post_disable,
what will happend?
Do you need to handle this?

BRs,
Rex

> +}
> +
>  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
>  	.attach = mtk_dsi_bridge_attach,
>  	.disable = mtk_dsi_bridge_disable,
>  	.enable = mtk_dsi_bridge_enable,
> +	.pre_enable = mtk_dsi_bridge_pre_enable,
> +	.post_disable = mtk_dsi_bridge_post_disable,
>  	.mode_set = mtk_dsi_bridge_mode_set,
>  };
>  

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > In order to match the changes of "Use the drm_panel_bridge API",
> > the poweron/poweroff of dsi is extracted from enable/disable and
> > defined as new funcs (pre_enable/post_disable).
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++---------
> > --
> > --
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 262c027d8c2f..e33caaca11a7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	if (--dsi->refcount != 0)
> >  		return;
> >  
> > -	/*
> > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > -	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > -	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > -	 * after dsi is fully set.
> > -	 */
> > -	mtk_dsi_stop(dsi);
> > -
> > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> >  {
> > -	int ret;
> > -
> >  	if (dsi->enabled)
> >  		return;
> >  
> > -	ret = mtk_dsi_poweron(dsi);
> > -	if (ret < 0) {
> > -		DRM_ERROR("failed to power on dsi\n");
> > -		return;
> > -	}
> > -
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > mtk_dsi *dsi)
> >  	if (!dsi->enabled)
> >  		return;
> >  
> > -	mtk_dsi_poweroff(dsi);
> > +	/*
> > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> 
> Why they are asymmetric?
> 
> > +	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > +	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > +	 * after dsi is fully set.
> > +	 */
> > +	mtk_dsi_stop(dsi);
> > +
> > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  
> >  	dsi->enabled = false;
> >  }
> > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > drm_bridge *bridge)
> >  	mtk_output_dsi_enable(dsi);
> >  }
> >  
> > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweron(dsi);
> 
> Should you handle the error of mtk_dsi_poweron?
> If you failed to mtk_dsi_bridge_pre_enable and do
> mtk_dsi_bridge_enable,
> what will happend?
> 
> > +}
> > +
> > +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweroff(dsi);
> 
> If you failed to mtk_dsi_bridge_disable and you do
> mtk_dsi_bridge_post_disable,
> what will happend?
> Do you need to handle this?
> 
> BRs,
> Rex
> 
> > +}
> > +
> >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> >  	.attach = mtk_dsi_bridge_attach,
> >  	.disable = mtk_dsi_bridge_disable,
> >  	.enable = mtk_dsi_bridge_enable,
> > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > +	.post_disable = mtk_dsi_bridge_post_disable,
> >  	.mode_set = mtk_dsi_bridge_mode_set,
> >  };
> >  
> 
> 

Hi Rex:

Thanks for your review!

1.Why they are asymmetric?
=>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
switch from LP11 and HS mode.DSI has two working modes:
If it is cmd mode, the data sent is sent by LP11, and dsi_start is just
a signal. In this mode, dsi_stop is not required after sending cmd.
If it is video mode, because the data needs to be sent in HS mode,
dsi_start is required to make dsi enter HS mode from LP11. After
suspend, drm will call dsi_disable, and call dsi_stop to make dsi
return from HS mode to LP11 state.
Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
For example, in the dsi_host_transfer function, only dsi_stop has no
dsi_start operation.

2.
Because the return type of pre_enable & post_disable in common code is
void type. If there is an error, it will be processed in
poweron/poweroff, and the error message will be printed.
Do you mean that pre_enable & post_disable needs to accept the
poweron/poweroff error return value and then print the error log?

3.
If pre_enable fails, there is only a problem with the dsi module, and
it does not affect the execution of other modules and enable funcs
under drm. 
Same goes for post_disable & disable.

Best Regrds!
xinlei

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > In order to match the changes of "Use the drm_panel_bridge API",
> > the poweron/poweroff of dsi is extracted from enable/disable and
> > defined as new funcs (pre_enable/post_disable).
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++---------
> > --
> > --
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 262c027d8c2f..e33caaca11a7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	if (--dsi->refcount != 0)
> >  		return;
> >  
> > -	/*
> > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > -	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > -	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > -	 * after dsi is fully set.
> > -	 */
> > -	mtk_dsi_stop(dsi);
> > -
> > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> >  {
> > -	int ret;
> > -
> >  	if (dsi->enabled)
> >  		return;
> >  
> > -	ret = mtk_dsi_poweron(dsi);
> > -	if (ret < 0) {
> > -		DRM_ERROR("failed to power on dsi\n");
> > -		return;
> > -	}
> > -
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > mtk_dsi *dsi)
> >  	if (!dsi->enabled)
> >  		return;
> >  
> > -	mtk_dsi_poweroff(dsi);
> > +	/*
> > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> 
> Why they are asymmetric?
> 
> > +	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > +	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > +	 * after dsi is fully set.
> > +	 */
> > +	mtk_dsi_stop(dsi);
> > +
> > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  
> >  	dsi->enabled = false;
> >  }
> > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > drm_bridge *bridge)
> >  	mtk_output_dsi_enable(dsi);
> >  }
> >  
> > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweron(dsi);
> 
> Should you handle the error of mtk_dsi_poweron?
> If you failed to mtk_dsi_bridge_pre_enable and do
> mtk_dsi_bridge_enable,
> what will happend?
> 
> > +}
> > +
> > +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweroff(dsi);
> 
> If you failed to mtk_dsi_bridge_disable and you do
> mtk_dsi_bridge_post_disable,
> what will happend?
> Do you need to handle this?
> 
> BRs,
> Rex
> 
> > +}
> > +
> >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> >  	.attach = mtk_dsi_bridge_attach,
> >  	.disable = mtk_dsi_bridge_disable,
> >  	.enable = mtk_dsi_bridge_enable,
> > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > +	.post_disable = mtk_dsi_bridge_post_disable,
> >  	.mode_set = mtk_dsi_bridge_mode_set,
> >  };
> >  
> 
> 

Hi Rex:

Thanks for your review!

1.Why they are asymmetric?
=>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
switch from LP11 and HS mode.DSI has two working modes:
If it is cmd mode, the data sent is sent by LP11, and dsi_start is just
a signal. In this mode, dsi_stop is not required after sending cmd.
If it is video mode, because the data needs to be sent in HS mode,
dsi_start is required to make dsi enter HS mode from LP11. After
suspend, drm will call dsi_disable, and call dsi_stop to make dsi
return from HS mode to LP11 state.
Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
For example, in the dsi_host_transfer function, only dsi_stop has no
dsi_start operation.

2.
Because the return type of pre_enable & post_disable in common code is
void type. If there is an error, it will be processed in
poweron/poweroff, and the error message will be printed.
Do you mean that pre_enable & post_disable needs to accept the
poweron/poweroff error return value and then print the error log?

3.
If pre_enable fails, there is only a problem with the dsi module, and
it does not affect the execution of other modules and enable funcs
under drm. 
Same goes for post_disable & disable.

Best Regrds!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > In order to match the changes of "Use the drm_panel_bridge API",
> > the poweron/poweroff of dsi is extracted from enable/disable and
> > defined as new funcs (pre_enable/post_disable).
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++---------
> > --
> > --
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 262c027d8c2f..e33caaca11a7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	if (--dsi->refcount != 0)
> >  		return;
> >  
> > -	/*
> > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > -	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > -	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > -	 * after dsi is fully set.
> > -	 */
> > -	mtk_dsi_stop(dsi);
> > -
> > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> >  {
> > -	int ret;
> > -
> >  	if (dsi->enabled)
> >  		return;
> >  
> > -	ret = mtk_dsi_poweron(dsi);
> > -	if (ret < 0) {
> > -		DRM_ERROR("failed to power on dsi\n");
> > -		return;
> > -	}
> > -
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > mtk_dsi *dsi)
> >  	if (!dsi->enabled)
> >  		return;
> >  
> > -	mtk_dsi_poweroff(dsi);
> > +	/*
> > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> 
> Why they are asymmetric?
> 
> > +	 * mtk_dsi_stop() should be called after
> > mtk_drm_crtc_atomic_disable(),
> > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > irq.
> > +	 * mtk_dsi_start() needs to be called in
> > mtk_output_dsi_enable(),
> > +	 * after dsi is fully set.
> > +	 */
> > +	mtk_dsi_stop(dsi);
> > +
> > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> >  
> >  	dsi->enabled = false;
> >  }
> > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > drm_bridge *bridge)
> >  	mtk_output_dsi_enable(dsi);
> >  }
> >  
> > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweron(dsi);
> 
> Should you handle the error of mtk_dsi_poweron?
> If you failed to mtk_dsi_bridge_pre_enable and do
> mtk_dsi_bridge_enable,
> what will happend?
> 
> > +}
> > +
> > +static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > +
> > +	mtk_dsi_poweroff(dsi);
> 
> If you failed to mtk_dsi_bridge_disable and you do
> mtk_dsi_bridge_post_disable,
> what will happend?
> Do you need to handle this?
> 
> BRs,
> Rex
> 
> > +}
> > +
> >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> >  	.attach = mtk_dsi_bridge_attach,
> >  	.disable = mtk_dsi_bridge_disable,
> >  	.enable = mtk_dsi_bridge_enable,
> > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > +	.post_disable = mtk_dsi_bridge_post_disable,
> >  	.mode_set = mtk_dsi_bridge_mode_set,
> >  };
> >  
> 
> 

Hi Rex:

Thanks for your review!

1.Why they are asymmetric?
=>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
switch from LP11 and HS mode.DSI has two working modes:
If it is cmd mode, the data sent is sent by LP11, and dsi_start is just
a signal. In this mode, dsi_stop is not required after sending cmd.
If it is video mode, because the data needs to be sent in HS mode,
dsi_start is required to make dsi enter HS mode from LP11. After
suspend, drm will call dsi_disable, and call dsi_stop to make dsi
return from HS mode to LP11 state.
Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
For example, in the dsi_host_transfer function, only dsi_stop has no
dsi_start operation.

2.
Because the return type of pre_enable & post_disable in common code is
void type. If there is an error, it will be processed in
poweron/poweroff, and the error message will be printed.
Do you mean that pre_enable & post_disable needs to accept the
poweron/poweroff error return value and then print the error log?

3.
If pre_enable fails, there is only a problem with the dsi module, and
it does not affect the execution of other modules and enable funcs
under drm. 
Same goes for post_disable & disable.

Best Regrds!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > 
> > > In order to match the changes of "Use the drm_panel_bridge API",
> > > the poweron/poweroff of dsi is extracted from enable/disable and
> > > defined as new funcs (pre_enable/post_disable).
> > > 
> > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > drm_panel_bridge
> > > API")
> > > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------
> > > --
> > > --
> > > --
> > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index 262c027d8c2f..e33caaca11a7 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	if (--dsi->refcount != 0)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > > -	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > -	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > -	 * after dsi is fully set.
> > > -	 */
> > > -	mtk_dsi_stop(dsi);
> > > -
> > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  
> > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > >  {
> > > -	int ret;
> > > -
> > >  	if (dsi->enabled)
> > >  		return;
> > >  
> > > -	ret = mtk_dsi_poweron(dsi);
> > > -	if (ret < 0) {
> > > -		DRM_ERROR("failed to power on dsi\n");
> > > -		return;
> > > -	}
> > > -
> > >  	mtk_dsi_set_mode(dsi);
> > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > >  
> > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > mtk_dsi *dsi)
> > >  	if (!dsi->enabled)
> > >  		return;
> > >  
> > > -	mtk_dsi_poweroff(dsi);
> > > +	/*
> > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > 
> > Why they are asymmetric?
> > 
> > > +	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > +	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > +	 * after dsi is fully set.
> > > +	 */
> > > +	mtk_dsi_stop(dsi);
> > > +
> > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  
> > >  	dsi->enabled = false;
> > >  }
> > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > drm_bridge *bridge)
> > >  	mtk_output_dsi_enable(dsi);
> > >  }
> > >  
> > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweron(dsi);
> > 
> > Should you handle the error of mtk_dsi_poweron?
> > If you failed to mtk_dsi_bridge_pre_enable and do
> > mtk_dsi_bridge_enable,
> > what will happend?
> > 
> > > +}
> > > +
> > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweroff(dsi);
> > 
> > If you failed to mtk_dsi_bridge_disable and you do
> > mtk_dsi_bridge_post_disable,
> > what will happend?
> > Do you need to handle this?
> > 
> > BRs,
> > Rex
> > 
> > > +}
> > > +
> > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > >  	.attach = mtk_dsi_bridge_attach,
> > >  	.disable = mtk_dsi_bridge_disable,
> > >  	.enable = mtk_dsi_bridge_enable,
> > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > >  };
> > >  
> > 
> > 
> 
> Hi Rex:
> 
> Thanks for your review!
> 
> 1.Why they are asymmetric?
> =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
> switch from LP11 and HS mode.DSI has two working modes:
> If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> just
> a signal. In this mode, dsi_stop is not required after sending cmd.
> If it is video mode, because the data needs to be sent in HS mode,
> dsi_start is required to make dsi enter HS mode from LP11. After
> suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> return from HS mode to LP11 state.
> Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> For example, in the dsi_host_transfer function, only dsi_stop has no
> dsi_start operation.
> 
> 2.
> Because the return type of pre_enable & post_disable in common code
> is
> void type. If there is an error, it will be processed in
> poweron/poweroff, and the error message will be printed.
> Do you mean that pre_enable & post_disable needs to accept the
> poweron/poweroff error return value and then print the error log?
> 
> 3.
> If pre_enable fails, there is only a problem with the dsi module, and
> it does not affect the execution of other modules and enable funcs
> under drm. 
> Same goes for post_disable & disable.
> 
> Best Regrds!
> xinlei
> 

Hello Xinlei,

about failure for mtk_dsi_poweron(), it is all becuase of error setting
for clock.
If we do not set clock correctly and not enable DSI, after set DSI
start, will it cause any issue? (maybe bus hang or something.)

Because in original drivers, if mtk_dsi_poweron failed, the DSI will
not start.

IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
and mtk_dsi_bridge_post_disable().

BRs,
Rex

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > 
> > > In order to match the changes of "Use the drm_panel_bridge API",
> > > the poweron/poweroff of dsi is extracted from enable/disable and
> > > defined as new funcs (pre_enable/post_disable).
> > > 
> > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > drm_panel_bridge
> > > API")
> > > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------
> > > --
> > > --
> > > --
> > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index 262c027d8c2f..e33caaca11a7 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	if (--dsi->refcount != 0)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > > -	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > -	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > -	 * after dsi is fully set.
> > > -	 */
> > > -	mtk_dsi_stop(dsi);
> > > -
> > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  
> > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > >  {
> > > -	int ret;
> > > -
> > >  	if (dsi->enabled)
> > >  		return;
> > >  
> > > -	ret = mtk_dsi_poweron(dsi);
> > > -	if (ret < 0) {
> > > -		DRM_ERROR("failed to power on dsi\n");
> > > -		return;
> > > -	}
> > > -
> > >  	mtk_dsi_set_mode(dsi);
> > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > >  
> > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > mtk_dsi *dsi)
> > >  	if (!dsi->enabled)
> > >  		return;
> > >  
> > > -	mtk_dsi_poweroff(dsi);
> > > +	/*
> > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > 
> > Why they are asymmetric?
> > 
> > > +	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > +	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > +	 * after dsi is fully set.
> > > +	 */
> > > +	mtk_dsi_stop(dsi);
> > > +
> > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  
> > >  	dsi->enabled = false;
> > >  }
> > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > drm_bridge *bridge)
> > >  	mtk_output_dsi_enable(dsi);
> > >  }
> > >  
> > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweron(dsi);
> > 
> > Should you handle the error of mtk_dsi_poweron?
> > If you failed to mtk_dsi_bridge_pre_enable and do
> > mtk_dsi_bridge_enable,
> > what will happend?
> > 
> > > +}
> > > +
> > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweroff(dsi);
> > 
> > If you failed to mtk_dsi_bridge_disable and you do
> > mtk_dsi_bridge_post_disable,
> > what will happend?
> > Do you need to handle this?
> > 
> > BRs,
> > Rex
> > 
> > > +}
> > > +
> > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > >  	.attach = mtk_dsi_bridge_attach,
> > >  	.disable = mtk_dsi_bridge_disable,
> > >  	.enable = mtk_dsi_bridge_enable,
> > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > >  };
> > >  
> > 
> > 
> 
> Hi Rex:
> 
> Thanks for your review!
> 
> 1.Why they are asymmetric?
> =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
> switch from LP11 and HS mode.DSI has two working modes:
> If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> just
> a signal. In this mode, dsi_stop is not required after sending cmd.
> If it is video mode, because the data needs to be sent in HS mode,
> dsi_start is required to make dsi enter HS mode from LP11. After
> suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> return from HS mode to LP11 state.
> Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> For example, in the dsi_host_transfer function, only dsi_stop has no
> dsi_start operation.
> 
> 2.
> Because the return type of pre_enable & post_disable in common code
> is
> void type. If there is an error, it will be processed in
> poweron/poweroff, and the error message will be printed.
> Do you mean that pre_enable & post_disable needs to accept the
> poweron/poweroff error return value and then print the error log?
> 
> 3.
> If pre_enable fails, there is only a problem with the dsi module, and
> it does not affect the execution of other modules and enable funcs
> under drm. 
> Same goes for post_disable & disable.
> 
> Best Regrds!
> xinlei
> 

Hello Xinlei,

about failure for mtk_dsi_poweron(), it is all becuase of error setting
for clock.
If we do not set clock correctly and not enable DSI, after set DSI
start, will it cause any issue? (maybe bus hang or something.)

Because in original drivers, if mtk_dsi_poweron failed, the DSI will
not start.

IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
and mtk_dsi_bridge_post_disable().

BRs,
Rex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[Rex-BC Chen]

On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > 
> > > In order to match the changes of "Use the drm_panel_bridge API",
> > > the poweron/poweroff of dsi is extracted from enable/disable and
> > > defined as new funcs (pre_enable/post_disable).
> > > 
> > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > drm_panel_bridge
> > > API")
> > > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------
> > > --
> > > --
> > > --
> > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index 262c027d8c2f..e33caaca11a7 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	if (--dsi->refcount != 0)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > > -	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > -	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > -	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > -	 * after dsi is fully set.
> > > -	 */
> > > -	mtk_dsi_stop(dsi);
> > > -
> > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  
> > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > >  {
> > > -	int ret;
> > > -
> > >  	if (dsi->enabled)
> > >  		return;
> > >  
> > > -	ret = mtk_dsi_poweron(dsi);
> > > -	if (ret < 0) {
> > > -		DRM_ERROR("failed to power on dsi\n");
> > > -		return;
> > > -	}
> > > -
> > >  	mtk_dsi_set_mode(dsi);
> > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > >  
> > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > mtk_dsi *dsi)
> > >  	if (!dsi->enabled)
> > >  		return;
> > >  
> > > -	mtk_dsi_poweroff(dsi);
> > > +	/*
> > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since
> > 
> > Why they are asymmetric?
> > 
> > > +	 * mtk_dsi_stop() should be called after
> > > mtk_drm_crtc_atomic_disable(),
> > > +	 * which needs irq for vblank, and mtk_dsi_stop() will disable
> > > irq.
> > > +	 * mtk_dsi_start() needs to be called in
> > > mtk_output_dsi_enable(),
> > > +	 * after dsi is fully set.
> > > +	 */
> > > +	mtk_dsi_stop(dsi);
> > > +
> > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > >  
> > >  	dsi->enabled = false;
> > >  }
> > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > drm_bridge *bridge)
> > >  	mtk_output_dsi_enable(dsi);
> > >  }
> > >  
> > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweron(dsi);
> > 
> > Should you handle the error of mtk_dsi_poweron?
> > If you failed to mtk_dsi_bridge_pre_enable and do
> > mtk_dsi_bridge_enable,
> > what will happend?
> > 
> > > +}
> > > +
> > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > *bridge)
> > > +{
> > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > +
> > > +	mtk_dsi_poweroff(dsi);
> > 
> > If you failed to mtk_dsi_bridge_disable and you do
> > mtk_dsi_bridge_post_disable,
> > what will happend?
> > Do you need to handle this?
> > 
> > BRs,
> > Rex
> > 
> > > +}
> > > +
> > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > >  	.attach = mtk_dsi_bridge_attach,
> > >  	.disable = mtk_dsi_bridge_disable,
> > >  	.enable = mtk_dsi_bridge_enable,
> > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > >  };
> > >  
> > 
> > 
> 
> Hi Rex:
> 
> Thanks for your review!
> 
> 1.Why they are asymmetric?
> =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi
> switch from LP11 and HS mode.DSI has two working modes:
> If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> just
> a signal. In this mode, dsi_stop is not required after sending cmd.
> If it is video mode, because the data needs to be sent in HS mode,
> dsi_start is required to make dsi enter HS mode from LP11. After
> suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> return from HS mode to LP11 state.
> Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> For example, in the dsi_host_transfer function, only dsi_stop has no
> dsi_start operation.
> 
> 2.
> Because the return type of pre_enable & post_disable in common code
> is
> void type. If there is an error, it will be processed in
> poweron/poweroff, and the error message will be printed.
> Do you mean that pre_enable & post_disable needs to accept the
> poweron/poweroff error return value and then print the error log?
> 
> 3.
> If pre_enable fails, there is only a problem with the dsi module, and
> it does not affect the execution of other modules and enable funcs
> under drm. 
> Same goes for post_disable & disable.
> 
> Best Regrds!
> xinlei
> 

Hello Xinlei,

about failure for mtk_dsi_poweron(), it is all becuase of error setting
for clock.
If we do not set clock correctly and not enable DSI, after set DSI
start, will it cause any issue? (maybe bus hang or something.)

Because in original drivers, if mtk_dsi_poweron failed, the DSI will
not start.

IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
and mtk_dsi_bridge_post_disable().

BRs,
Rex


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

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

[xinlei.lee]

On Wed, 2022-03-23 at 19:46 +0800, Rex-BC Chen wrote:
> On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
> > On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
> > > Hello Xinlei,
> > > 
> > > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > > From: Jitao Shi <jitao.shi@mediatek.com>
> > > > 
> > > > In order to match the changes of "Use the drm_panel_bridge
> > > > API",
> > > > the poweron/poweroff of dsi is extracted from enable/disable
> > > > and
> > > > defined as new funcs (pre_enable/post_disable).
> > > > 
> > > > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > > > drm_panel_bridge
> > > > API")
> > > > 
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > ---
> > > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-----
> > > > --
> > > > --
> > > > --
> > > > --
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > index 262c027d8c2f..e33caaca11a7 100644
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > > @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  	if (--dsi->refcount != 0)
> > > >  		return;
> > > >  
> > > > -	/*
> > > > -	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > > -	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > -	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > -	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > -	 * after dsi is fully set.
> > > > -	 */
> > > > -	mtk_dsi_stop(dsi);
> > > > -
> > > > -	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  	mtk_dsi_reset_engine(dsi);
> > > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > > @@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct
> > > > mtk_dsi
> > > > *dsi)
> > > >  
> > > >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > > >  {
> > > > -	int ret;
> > > > -
> > > >  	if (dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	ret = mtk_dsi_poweron(dsi);
> > > > -	if (ret < 0) {
> > > > -		DRM_ERROR("failed to power on dsi\n");
> > > > -		return;
> > > > -	}
> > > > -
> > > >  	mtk_dsi_set_mode(dsi);
> > > >  	mtk_dsi_clk_hs_mode(dsi, 1);
> > > >  
> > > > @@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct
> > > > mtk_dsi *dsi)
> > > >  	if (!dsi->enabled)
> > > >  		return;
> > > >  
> > > > -	mtk_dsi_poweroff(dsi);
> > > > +	/*
> > > > +	 * mtk_dsi_stop() and mtk_dsi_start() is asymmetric,
> > > > since
> > > 
> > > Why they are asymmetric?
> > > 
> > > > +	 * mtk_dsi_stop() should be called after
> > > > mtk_drm_crtc_atomic_disable(),
> > > > +	 * which needs irq for vblank, and mtk_dsi_stop() will
> > > > disable
> > > > irq.
> > > > +	 * mtk_dsi_start() needs to be called in
> > > > mtk_output_dsi_enable(),
> > > > +	 * after dsi is fully set.
> > > > +	 */
> > > > +	mtk_dsi_stop(dsi);
> > > > +
> > > > +	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
> > > >  
> > > >  	dsi->enabled = false;
> > > >  }
> > > > @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct
> > > > drm_bridge *bridge)
> > > >  	mtk_output_dsi_enable(dsi);
> > > >  }
> > > >  
> > > > +static void mtk_dsi_bridge_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweron(dsi);
> > > 
> > > Should you handle the error of mtk_dsi_poweron?
> > > If you failed to mtk_dsi_bridge_pre_enable and do
> > > mtk_dsi_bridge_enable,
> > > what will happend?
> > > 
> > > > +}
> > > > +
> > > > +static void mtk_dsi_bridge_post_disable(struct drm_bridge
> > > > *bridge)
> > > > +{
> > > > +	struct mtk_dsi *dsi = bridge_to_dsi(bridge);
> > > > +
> > > > +	mtk_dsi_poweroff(dsi);
> > > 
> > > If you failed to mtk_dsi_bridge_disable and you do
> > > mtk_dsi_bridge_post_disable,
> > > what will happend?
> > > Do you need to handle this?
> > > 
> > > BRs,
> > > Rex
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
> > > >  	.attach = mtk_dsi_bridge_attach,
> > > >  	.disable = mtk_dsi_bridge_disable,
> > > >  	.enable = mtk_dsi_bridge_enable,
> > > > +	.pre_enable = mtk_dsi_bridge_pre_enable,
> > > > +	.post_disable = mtk_dsi_bridge_post_disable,
> > > >  	.mode_set = mtk_dsi_bridge_mode_set,
> > > >  };
> > > >  
> > > 
> > > 
> > 
> > Hi Rex:
> > 
> > Thanks for your review!
> > 
> > 1.Why they are asymmetric?
> > =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make
> > dsi
> > switch from LP11 and HS mode.DSI has two working modes:
> > If it is cmd mode, the data sent is sent by LP11, and dsi_start is
> > just
> > a signal. In this mode, dsi_stop is not required after sending cmd.
> > If it is video mode, because the data needs to be sent in HS mode,
> > dsi_start is required to make dsi enter HS mode from LP11. After
> > suspend, drm will call dsi_disable, and call dsi_stop to make dsi
> > return from HS mode to LP11 state.
> > Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric.
> > For example, in the dsi_host_transfer function, only dsi_stop has
> > no
> > dsi_start operation.
> > 
> > 2.
> > Because the return type of pre_enable & post_disable in common code
> > is
> > void type. If there is an error, it will be processed in
> > poweron/poweroff, and the error message will be printed.
> > Do you mean that pre_enable & post_disable needs to accept the
> > poweron/poweroff error return value and then print the error log?
> > 
> > 3.
> > If pre_enable fails, there is only a problem with the dsi module,
> > and
> > it does not affect the execution of other modules and enable funcs
> > under drm. 
> > Same goes for post_disable & disable.
> > 
> > Best Regrds!
> > xinlei
> > 
> 
> Hello Xinlei,
> 
> about failure for mtk_dsi_poweron(), it is all becuase of error
> setting
> for clock.
> If we do not set clock correctly and not enable DSI, after set DSI
> start, will it cause any issue? (maybe bus hang or something.)
> 
> Because in original drivers, if mtk_dsi_poweron failed, the DSI will
> not start.
> 
> IMO, it also needs some error message for mtk_dsi_bridge_pre_enable()
> and mtk_dsi_bridge_post_disable().
> 
> BRs,
> Rex
> 

Hi Rex:

Thanks for your review.
In the original dsi_poweron, if (++dsi->refcount != 1) is used to
determine whether poweron has been repeatedly run.
The poweron failure to execute DSI Start you mentioned may indeed lead
to Bus hang in extreme cases, so I will add a poweron-like judgment
mechanism before the next version of enable.
And will add some error message for mtk_dsi_bridge_pre_enable() and
mtk_dsi_bridge_post_disable().

Best Regards!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

To comply with the panel sequence, hold the mipi signal to LP00 before the dcs cmds transmission,
and pull the mipi signal high from LP00 to LP11 until the start of the dcs cmds transmission.
If dsi is not in cmd mode, then dsi will pull the mipi signal high in the mtk_output_dsi_enable function.

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index e33caaca11a7..b509d59235e2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -203,6 +203,7 @@ struct mtk_dsi {
 	struct mtk_phy_timing phy_timing;
 	int refcount;
 	bool enabled;
+	bool lanes_ready;
 	u32 irq_data;
 	wait_queue_head_t irq_wait_queue;
 	const struct mtk_dsi_driver_data *driver_data;
@@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
-	mtk_dsi_clk_ulp_mode_leave(dsi);
-	mtk_dsi_lane0_ulp_mode_leave(dsi);
-	mtk_dsi_clk_hs_mode(dsi, 0);
-
 	return 0;
 err_disable_engine_clk:
 	clk_disable_unprepare(dsi->engine_clk);
@@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	clk_disable_unprepare(dsi->digital_clk);
 
 	phy_power_off(dsi->phy);
+
+	dsi->lanes_ready = false;
 }
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
@@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 	if (dsi->enabled)
 		return;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+	}
+
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (MTK_DSI_HOST_IS_READ(msg->type))
 		irq_flag |= LPRX_RD_RDY_INT_FLAG;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+		msleep(20);
+	}
+
 	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
 	if (ret)
 		goto restore_dsi_mode;
-- 
2.18.0

[PATCH v3, 3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

To comply with the panel sequence, hold the mipi signal to LP00 before the dcs cmds transmission,
and pull the mipi signal high from LP00 to LP11 until the start of the dcs cmds transmission.
If dsi is not in cmd mode, then dsi will pull the mipi signal high in the mtk_output_dsi_enable function.

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index e33caaca11a7..b509d59235e2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -203,6 +203,7 @@ struct mtk_dsi {
 	struct mtk_phy_timing phy_timing;
 	int refcount;
 	bool enabled;
+	bool lanes_ready;
 	u32 irq_data;
 	wait_queue_head_t irq_wait_queue;
 	const struct mtk_dsi_driver_data *driver_data;
@@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
-	mtk_dsi_clk_ulp_mode_leave(dsi);
-	mtk_dsi_lane0_ulp_mode_leave(dsi);
-	mtk_dsi_clk_hs_mode(dsi, 0);
-
 	return 0;
 err_disable_engine_clk:
 	clk_disable_unprepare(dsi->engine_clk);
@@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	clk_disable_unprepare(dsi->digital_clk);
 
 	phy_power_off(dsi->phy);
+
+	dsi->lanes_ready = false;
 }
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
@@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 	if (dsi->enabled)
 		return;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+	}
+
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (MTK_DSI_HOST_IS_READ(msg->type))
 		irq_flag |= LPRX_RD_RDY_INT_FLAG;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+		msleep(20);
+	}
+
 	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
 	if (ret)
 		goto restore_dsi_mode;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3, 3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

To comply with the panel sequence, hold the mipi signal to LP00 before the dcs cmds transmission,
and pull the mipi signal high from LP00 to LP11 until the start of the dcs cmds transmission.
If dsi is not in cmd mode, then dsi will pull the mipi signal high in the mtk_output_dsi_enable function.

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index e33caaca11a7..b509d59235e2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -203,6 +203,7 @@ struct mtk_dsi {
 	struct mtk_phy_timing phy_timing;
 	int refcount;
 	bool enabled;
+	bool lanes_ready;
 	u32 irq_data;
 	wait_queue_head_t irq_wait_queue;
 	const struct mtk_dsi_driver_data *driver_data;
@@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
-	mtk_dsi_clk_ulp_mode_leave(dsi);
-	mtk_dsi_lane0_ulp_mode_leave(dsi);
-	mtk_dsi_clk_hs_mode(dsi, 0);
-
 	return 0;
 err_disable_engine_clk:
 	clk_disable_unprepare(dsi->engine_clk);
@@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	clk_disable_unprepare(dsi->digital_clk);
 
 	phy_power_off(dsi->phy);
+
+	dsi->lanes_ready = false;
 }
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
@@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 	if (dsi->enabled)
 		return;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+	}
+
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (MTK_DSI_HOST_IS_READ(msg->type))
 		irq_flag |= LPRX_RD_RDY_INT_FLAG;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+		msleep(20);
+	}
+
 	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
 	if (ret)
 		goto restore_dsi_mode;
-- 
2.18.0


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

[PATCH v3, 3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

From: Jitao Shi <jitao.shi@mediatek.com>

To comply with the panel sequence, hold the mipi signal to LP00 before the dcs cmds transmission,
and pull the mipi signal high from LP00 to LP11 until the start of the dcs cmds transmission.
If dsi is not in cmd mode, then dsi will pull the mipi signal high in the mtk_output_dsi_enable function.

Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index e33caaca11a7..b509d59235e2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -203,6 +203,7 @@ struct mtk_dsi {
 	struct mtk_phy_timing phy_timing;
 	int refcount;
 	bool enabled;
+	bool lanes_ready;
 	u32 irq_data;
 	wait_queue_head_t irq_wait_queue;
 	const struct mtk_dsi_driver_data *driver_data;
@@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_config_vdo_timing(dsi);
 	mtk_dsi_set_interrupt_enable(dsi);
 
-	mtk_dsi_rxtx_control(dsi);
-	usleep_range(30, 100);
-	mtk_dsi_reset_dphy(dsi);
-	mtk_dsi_clk_ulp_mode_leave(dsi);
-	mtk_dsi_lane0_ulp_mode_leave(dsi);
-	mtk_dsi_clk_hs_mode(dsi, 0);
-
 	return 0;
 err_disable_engine_clk:
 	clk_disable_unprepare(dsi->engine_clk);
@@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	clk_disable_unprepare(dsi->digital_clk);
 
 	phy_power_off(dsi->phy);
+
+	dsi->lanes_ready = false;
 }
 
 static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
@@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 	if (dsi->enabled)
 		return;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+	}
+
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
 
@@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (MTK_DSI_HOST_IS_READ(msg->type))
 		irq_flag |= LPRX_RD_RDY_INT_FLAG;
 
+	if (!dsi->lanes_ready) {
+		dsi->lanes_ready = true;
+		mtk_dsi_rxtx_control(dsi);
+		usleep_range(30, 100);
+		mtk_dsi_reset_dphy(dsi);
+		mtk_dsi_clk_ulp_mode_leave(dsi);
+		mtk_dsi_lane0_ulp_mode_leave(dsi);
+		mtk_dsi_clk_hs_mode(dsi, 0);
+		msleep(20);
+	}
+
 	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
 	if (ret)
 		goto restore_dsi_mode;
-- 
2.18.0

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> To comply with the panel sequence, hold the mipi signal to LP00 

Could you provide a example panel power sequence to let me understand
that?
Maybe you can put them in commit message.

> before the dcs cmds transmission,
> and pull the mipi signal high from LP00 to LP11 until the start of
> the dcs cmds transmission.

Maybe you can try to write as:
To comply with...
- Hold the mipi signal...
- Pul the miip signal high....

> If dsi is not in cmd mode, then dsi will pull the mipi signal high in
> the mtk_output_dsi_enable function.
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 

Can you remove this blank line?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-----
> --
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e33caaca11a7..b509d59235e2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -203,6 +203,7 @@ struct mtk_dsi {
>  	struct mtk_phy_timing phy_timing;
>  	int refcount;
>  	bool enabled;
> +	bool lanes_ready;
>  	u32 irq_data;
>  	wait_queue_head_t irq_wait_queue;
>  	const struct mtk_dsi_driver_data *driver_data;
> @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
> -	mtk_dsi_clk_ulp_mode_leave(dsi);
> -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> -	mtk_dsi_clk_hs_mode(dsi, 0);
> -
>  	return 0;
>  err_disable_engine_clk:
>  	clk_disable_unprepare(dsi->engine_clk);
> @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	clk_disable_unprepare(dsi->digital_clk);
>  
>  	phy_power_off(dsi->phy);
> +
> +	dsi->lanes_ready = false;
>  }
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi
> *dsi)
>  	if (dsi->enabled)
>  		return;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);
> +	}
> +
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> mipi_dsi_host *host,
>  	if (MTK_DSI_HOST_IS_READ(msg->type))
>  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);

The drivers are the same with previous modification.
I think you can use a funtion for them?

> +		msleep(20);

Why delay 20ms but not in mtk_output_dsi_enable?

BRs,
Rex
> +	}
> +
>  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
>  	if (ret)
>  		goto restore_dsi_mode;

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> To comply with the panel sequence, hold the mipi signal to LP00 

Could you provide a example panel power sequence to let me understand
that?
Maybe you can put them in commit message.

> before the dcs cmds transmission,
> and pull the mipi signal high from LP00 to LP11 until the start of
> the dcs cmds transmission.

Maybe you can try to write as:
To comply with...
- Hold the mipi signal...
- Pul the miip signal high....

> If dsi is not in cmd mode, then dsi will pull the mipi signal high in
> the mtk_output_dsi_enable function.
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 

Can you remove this blank line?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-----
> --
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e33caaca11a7..b509d59235e2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -203,6 +203,7 @@ struct mtk_dsi {
>  	struct mtk_phy_timing phy_timing;
>  	int refcount;
>  	bool enabled;
> +	bool lanes_ready;
>  	u32 irq_data;
>  	wait_queue_head_t irq_wait_queue;
>  	const struct mtk_dsi_driver_data *driver_data;
> @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
> -	mtk_dsi_clk_ulp_mode_leave(dsi);
> -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> -	mtk_dsi_clk_hs_mode(dsi, 0);
> -
>  	return 0;
>  err_disable_engine_clk:
>  	clk_disable_unprepare(dsi->engine_clk);
> @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	clk_disable_unprepare(dsi->digital_clk);
>  
>  	phy_power_off(dsi->phy);
> +
> +	dsi->lanes_ready = false;
>  }
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi
> *dsi)
>  	if (dsi->enabled)
>  		return;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);
> +	}
> +
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> mipi_dsi_host *host,
>  	if (MTK_DSI_HOST_IS_READ(msg->type))
>  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);

The drivers are the same with previous modification.
I think you can use a funtion for them?

> +		msleep(20);

Why delay 20ms but not in mtk_output_dsi_enable?

BRs,
Rex
> +	}
> +
>  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
>  	if (ret)
>  		goto restore_dsi_mode;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> To comply with the panel sequence, hold the mipi signal to LP00 

Could you provide a example panel power sequence to let me understand
that?
Maybe you can put them in commit message.

> before the dcs cmds transmission,
> and pull the mipi signal high from LP00 to LP11 until the start of
> the dcs cmds transmission.

Maybe you can try to write as:
To comply with...
- Hold the mipi signal...
- Pul the miip signal high....

> If dsi is not in cmd mode, then dsi will pull the mipi signal high in
> the mtk_output_dsi_enable function.
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 

Can you remove this blank line?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-----
> --
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e33caaca11a7..b509d59235e2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -203,6 +203,7 @@ struct mtk_dsi {
>  	struct mtk_phy_timing phy_timing;
>  	int refcount;
>  	bool enabled;
> +	bool lanes_ready;
>  	u32 irq_data;
>  	wait_queue_head_t irq_wait_queue;
>  	const struct mtk_dsi_driver_data *driver_data;
> @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
> -	mtk_dsi_clk_ulp_mode_leave(dsi);
> -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> -	mtk_dsi_clk_hs_mode(dsi, 0);
> -
>  	return 0;
>  err_disable_engine_clk:
>  	clk_disable_unprepare(dsi->engine_clk);
> @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	clk_disable_unprepare(dsi->digital_clk);
>  
>  	phy_power_off(dsi->phy);
> +
> +	dsi->lanes_ready = false;
>  }
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi
> *dsi)
>  	if (dsi->enabled)
>  		return;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);
> +	}
> +
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> mipi_dsi_host *host,
>  	if (MTK_DSI_HOST_IS_READ(msg->type))
>  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);

The drivers are the same with previous modification.
I think you can use a funtion for them?

> +		msleep(20);

Why delay 20ms but not in mtk_output_dsi_enable?

BRs,
Rex
> +	}
> +
>  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
>  	if (ret)
>  		goto restore_dsi_mode;


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

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Jitao Shi <jitao.shi@mediatek.com>
> 
> To comply with the panel sequence, hold the mipi signal to LP00 

Could you provide a example panel power sequence to let me understand
that?
Maybe you can put them in commit message.

> before the dcs cmds transmission,
> and pull the mipi signal high from LP00 to LP11 until the start of
> the dcs cmds transmission.

Maybe you can try to write as:
To comply with...
- Hold the mipi signal...
- Pul the miip signal high....

> If dsi is not in cmd mode, then dsi will pull the mipi signal high in
> the mtk_output_dsi_enable function.
> 
> Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge
> API")
> 

Can you remove this blank line?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++-----
> --
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e33caaca11a7..b509d59235e2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -203,6 +203,7 @@ struct mtk_dsi {
>  	struct mtk_phy_timing phy_timing;
>  	int refcount;
>  	bool enabled;
> +	bool lanes_ready;
>  	u32 irq_data;
>  	wait_queue_head_t irq_wait_queue;
>  	const struct mtk_dsi_driver_data *driver_data;
> @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  	mtk_dsi_config_vdo_timing(dsi);
>  	mtk_dsi_set_interrupt_enable(dsi);
>  
> -	mtk_dsi_rxtx_control(dsi);
> -	usleep_range(30, 100);
> -	mtk_dsi_reset_dphy(dsi);
> -	mtk_dsi_clk_ulp_mode_leave(dsi);
> -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> -	mtk_dsi_clk_hs_mode(dsi, 0);
> -
>  	return 0;
>  err_disable_engine_clk:
>  	clk_disable_unprepare(dsi->engine_clk);
> @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	clk_disable_unprepare(dsi->digital_clk);
>  
>  	phy_power_off(dsi->phy);
> +
> +	dsi->lanes_ready = false;
>  }
>  
>  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct mtk_dsi
> *dsi)
>  	if (dsi->enabled)
>  		return;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);
> +	}
> +
>  	mtk_dsi_set_mode(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);
>  
> @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> mipi_dsi_host *host,
>  	if (MTK_DSI_HOST_IS_READ(msg->type))
>  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
>  
> +	if (!dsi->lanes_ready) {
> +		dsi->lanes_ready = true;
> +		mtk_dsi_rxtx_control(dsi);
> +		usleep_range(30, 100);
> +		mtk_dsi_reset_dphy(dsi);
> +		mtk_dsi_clk_ulp_mode_leave(dsi);
> +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> +		mtk_dsi_clk_hs_mode(dsi, 0);

The drivers are the same with previous modification.
I think you can use a funtion for them?

> +		msleep(20);

Why delay 20ms but not in mtk_output_dsi_enable?

BRs,
Rex
> +	}
> +
>  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
>  	if (ret)
>  		goto restore_dsi_mode;

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

On Thu, 2022-03-17 at 20:06 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > To comply with the panel sequence, hold the mipi signal to LP00 
> 
> Could you provide a example panel power sequence to let me understand
> that?
> Maybe you can put them in commit message.
> 
> > before the dcs cmds transmission,
> > and pull the mipi signal high from LP00 to LP11 until the start of
> > the dcs cmds transmission.
> 
> Maybe you can try to write as:
> To comply with...
> - Hold the mipi signal...
> - Pul the miip signal high....
> 
> > If dsi is not in cmd mode, then dsi will pull the mipi signal high
> > in
> > the mtk_output_dsi_enable function.
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> 
> Can you remove this blank line?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++---
> > --
> > --
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index e33caaca11a7..b509d59235e2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -203,6 +203,7 @@ struct mtk_dsi {
> >  	struct mtk_phy_timing phy_timing;
> >  	int refcount;
> >  	bool enabled;
> > +	bool lanes_ready;
> >  	u32 irq_data;
> >  	wait_queue_head_t irq_wait_queue;
> >  	const struct mtk_dsi_driver_data *driver_data;
> > @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> > -	mtk_dsi_clk_ulp_mode_leave(dsi);
> > -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> > -	mtk_dsi_clk_hs_mode(dsi, 0);
> > -
> >  	return 0;
> >  err_disable_engine_clk:
> >  	clk_disable_unprepare(dsi->engine_clk);
> > @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	clk_disable_unprepare(dsi->digital_clk);
> >  
> >  	phy_power_off(dsi->phy);
> > +
> > +	dsi->lanes_ready = false;
> >  }
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct
> > mtk_dsi
> > *dsi)
> >  	if (dsi->enabled)
> >  		return;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> > +	}
> > +
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> > mipi_dsi_host *host,
> >  	if (MTK_DSI_HOST_IS_READ(msg->type))
> >  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> 
> The drivers are the same with previous modification.
> I think you can use a funtion for them?
> 
> > +		msleep(20);
> 
> Why delay 20ms but not in mtk_output_dsi_enable?
> 
> BRs,
> Rex
> > +	}
> > +
> >  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
> >  	if (ret)
> >  		goto restore_dsi_mode;
> 
> 

Hi rex:

Thanks for your review.

1.The normal panel timing is as follows (I will add to the commit
message in the next version):
(1) pp1800 DC pull up
(2) avdd & avee AC pull high
(3) lcm_reset pull high -> pull low -> pull high
(4) Pull MIPI signal high (LP11) -> initial code -> send video data(HS
mode)
The power-off sequence is reversed.

2.
The 20ms delay in dsi_host_transfer is because dsi needs to give
dsi_rx(panel) a reaction time after pulling up the mipi signal.
If you suggest encapsulating it as a function I will add a delay to
mtk_output_dsi_enable in the next version as well.
Best Regards!
xinlei

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

On Thu, 2022-03-17 at 20:06 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > To comply with the panel sequence, hold the mipi signal to LP00 
> 
> Could you provide a example panel power sequence to let me understand
> that?
> Maybe you can put them in commit message.
> 
> > before the dcs cmds transmission,
> > and pull the mipi signal high from LP00 to LP11 until the start of
> > the dcs cmds transmission.
> 
> Maybe you can try to write as:
> To comply with...
> - Hold the mipi signal...
> - Pul the miip signal high....
> 
> > If dsi is not in cmd mode, then dsi will pull the mipi signal high
> > in
> > the mtk_output_dsi_enable function.
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> 
> Can you remove this blank line?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++---
> > --
> > --
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index e33caaca11a7..b509d59235e2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -203,6 +203,7 @@ struct mtk_dsi {
> >  	struct mtk_phy_timing phy_timing;
> >  	int refcount;
> >  	bool enabled;
> > +	bool lanes_ready;
> >  	u32 irq_data;
> >  	wait_queue_head_t irq_wait_queue;
> >  	const struct mtk_dsi_driver_data *driver_data;
> > @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> > -	mtk_dsi_clk_ulp_mode_leave(dsi);
> > -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> > -	mtk_dsi_clk_hs_mode(dsi, 0);
> > -
> >  	return 0;
> >  err_disable_engine_clk:
> >  	clk_disable_unprepare(dsi->engine_clk);
> > @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	clk_disable_unprepare(dsi->digital_clk);
> >  
> >  	phy_power_off(dsi->phy);
> > +
> > +	dsi->lanes_ready = false;
> >  }
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct
> > mtk_dsi
> > *dsi)
> >  	if (dsi->enabled)
> >  		return;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> > +	}
> > +
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> > mipi_dsi_host *host,
> >  	if (MTK_DSI_HOST_IS_READ(msg->type))
> >  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> 
> The drivers are the same with previous modification.
> I think you can use a funtion for them?
> 
> > +		msleep(20);
> 
> Why delay 20ms but not in mtk_output_dsi_enable?
> 
> BRs,
> Rex
> > +	}
> > +
> >  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
> >  	if (ret)
> >  		goto restore_dsi_mode;
> 
> 

Hi rex:

Thanks for your review.

1.The normal panel timing is as follows (I will add to the commit
message in the next version):
(1) pp1800 DC pull up
(2) avdd & avee AC pull high
(3) lcm_reset pull high -> pull low -> pull high
(4) Pull MIPI signal high (LP11) -> initial code -> send video data(HS
mode)
The power-off sequence is reversed.

2.
The 20ms delay in dsi_host_transfer is because dsi needs to give
dsi_rx(panel) a reaction time after pulling up the mipi signal.
If you suggest encapsulating it as a function I will add a delay to
mtk_output_dsi_enable in the next version as well.
Best Regards!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,3/4] drm/mediatek: keep dsi as LP00 before dcs cmds transfer

[xinlei.lee]

On Thu, 2022-03-17 at 20:06 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Jitao Shi <jitao.shi@mediatek.com>
> > 
> > To comply with the panel sequence, hold the mipi signal to LP00 
> 
> Could you provide a example panel power sequence to let me understand
> that?
> Maybe you can put them in commit message.
> 
> > before the dcs cmds transmission,
> > and pull the mipi signal high from LP00 to LP11 until the start of
> > the dcs cmds transmission.
> 
> Maybe you can try to write as:
> To comply with...
> - Hold the mipi signal...
> - Pul the miip signal high....
> 
> > If dsi is not in cmd mode, then dsi will pull the mipi signal high
> > in
> > the mtk_output_dsi_enable function.
> > 
> > Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the
> > drm_panel_bridge
> > API")
> > 
> 
> Can you remove this blank line?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 31 +++++++++++++++++++++++---
> > --
> > --
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index e33caaca11a7..b509d59235e2 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -203,6 +203,7 @@ struct mtk_dsi {
> >  	struct mtk_phy_timing phy_timing;
> >  	int refcount;
> >  	bool enabled;
> > +	bool lanes_ready;
> >  	u32 irq_data;
> >  	wait_queue_head_t irq_wait_queue;
> >  	const struct mtk_dsi_driver_data *driver_data;
> > @@ -654,13 +655,6 @@ static int mtk_dsi_poweron(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_config_vdo_timing(dsi);
> >  	mtk_dsi_set_interrupt_enable(dsi);
> >  
> > -	mtk_dsi_rxtx_control(dsi);
> > -	usleep_range(30, 100);
> > -	mtk_dsi_reset_dphy(dsi);
> > -	mtk_dsi_clk_ulp_mode_leave(dsi);
> > -	mtk_dsi_lane0_ulp_mode_leave(dsi);
> > -	mtk_dsi_clk_hs_mode(dsi, 0);
> > -
> >  	return 0;
> >  err_disable_engine_clk:
> >  	clk_disable_unprepare(dsi->engine_clk);
> > @@ -689,6 +683,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	clk_disable_unprepare(dsi->digital_clk);
> >  
> >  	phy_power_off(dsi->phy);
> > +
> > +	dsi->lanes_ready = false;
> >  }
> >  
> >  static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > @@ -696,6 +692,16 @@ static void mtk_output_dsi_enable(struct
> > mtk_dsi
> > *dsi)
> >  	if (dsi->enabled)
> >  		return;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> > +	}
> > +
> >  	mtk_dsi_set_mode(dsi);
> >  	mtk_dsi_clk_hs_mode(dsi, 1);
> >  
> > @@ -995,6 +1001,17 @@ static ssize_t mtk_dsi_host_transfer(struct
> > mipi_dsi_host *host,
> >  	if (MTK_DSI_HOST_IS_READ(msg->type))
> >  		irq_flag |= LPRX_RD_RDY_INT_FLAG;
> >  
> > +	if (!dsi->lanes_ready) {
> > +		dsi->lanes_ready = true;
> > +		mtk_dsi_rxtx_control(dsi);
> > +		usleep_range(30, 100);
> > +		mtk_dsi_reset_dphy(dsi);
> > +		mtk_dsi_clk_ulp_mode_leave(dsi);
> > +		mtk_dsi_lane0_ulp_mode_leave(dsi);
> > +		mtk_dsi_clk_hs_mode(dsi, 0);
> 
> The drivers are the same with previous modification.
> I think you can use a funtion for them?
> 
> > +		msleep(20);
> 
> Why delay 20ms but not in mtk_output_dsi_enable?
> 
> BRs,
> Rex
> > +	}
> > +
> >  	ret = mtk_dsi_host_send_cmd(dsi, msg, irq_flag);
> >  	if (ret)
> >  		goto restore_dsi_mode;
> 
> 

Hi rex:

Thanks for your review.

1.The normal panel timing is as follows (I will add to the commit
message in the next version):
(1) pp1800 DC pull up
(2) avdd & avee AC pull high
(3) lcm_reset pull high -> pull low -> pull high
(4) Pull MIPI signal high (LP11) -> initial code -> send video data(HS
mode)
The power-off sequence is reversed.

2.
The 20ms delay in dsi_host_transfer is because dsi needs to give
dsi_rx(panel) a reaction time after pulling up the mipi signal.
If you suggest encapsulating it as a function I will add a delay to
mtk_output_dsi_enable in the next version as well.
Best Regards!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In the dsi_enable function, mtk_dsi_rxtx_control is to
pull up the MIPI signal operation. Before dsi_disable,
MIPI should also be pulled down by writing a register instead of disabling dsi.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b509d59235e2..1c6a75a46b67 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
+	/* set the lane number as 0 */
+	writel(0, dsi->regs + DSI_TXRX_CTRL);
 
 	mtk_dsi_disable(dsi);
 
-- 
2.18.0

[PATCH v3, 4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In the dsi_enable function, mtk_dsi_rxtx_control is to
pull up the MIPI signal operation. Before dsi_disable,
MIPI should also be pulled down by writing a register instead of disabling dsi.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b509d59235e2..1c6a75a46b67 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
+	/* set the lane number as 0 */
+	writel(0, dsi->regs + DSI_TXRX_CTRL);
 
 	mtk_dsi_disable(dsi);
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3, 4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In the dsi_enable function, mtk_dsi_rxtx_control is to
pull up the MIPI signal operation. Before dsi_disable,
MIPI should also be pulled down by writing a register instead of disabling dsi.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b509d59235e2..1c6a75a46b67 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
+	/* set the lane number as 0 */
+	writel(0, dsi->regs + DSI_TXRX_CTRL);
 
 	mtk_dsi_disable(dsi);
 
-- 
2.18.0


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

[PATCH v3, 4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

In the dsi_enable function, mtk_dsi_rxtx_control is to
pull up the MIPI signal operation. Before dsi_disable,
MIPI should also be pulled down by writing a register instead of disabling dsi.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b509d59235e2..1c6a75a46b67 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);
+	/* set the lane number as 0 */
+	writel(0, dsi->regs + DSI_TXRX_CTRL);
 
 	mtk_dsi_disable(dsi);
 
-- 
2.18.0

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> In the dsi_enable function, mtk_dsi_rxtx_control is to
> pull up the MIPI signal operation. Before dsi_disable,
> MIPI should also be pulled down by writing a register instead of
> disabling dsi.
> 

What will happen if you do not pulled down the mipi before disable dsi?
What's differnet for this two setting?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index b509d59235e2..1c6a75a46b67 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> +	/* set the lane number as 0 */
> +	writel(0, dsi->regs + DSI_TXRX_CTRL);

So set lane num to 0 means pull down mipi?

BRs,
Rex

>  
>  	mtk_dsi_disable(dsi);
>  

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> In the dsi_enable function, mtk_dsi_rxtx_control is to
> pull up the MIPI signal operation. Before dsi_disable,
> MIPI should also be pulled down by writing a register instead of
> disabling dsi.
> 

What will happen if you do not pulled down the mipi before disable dsi?
What's differnet for this two setting?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index b509d59235e2..1c6a75a46b67 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> +	/* set the lane number as 0 */
> +	writel(0, dsi->regs + DSI_TXRX_CTRL);

So set lane num to 0 means pull down mipi?

BRs,
Rex

>  
>  	mtk_dsi_disable(dsi);
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> In the dsi_enable function, mtk_dsi_rxtx_control is to
> pull up the MIPI signal operation. Before dsi_disable,
> MIPI should also be pulled down by writing a register instead of
> disabling dsi.
> 

What will happen if you do not pulled down the mipi before disable dsi?
What's differnet for this two setting?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index b509d59235e2..1c6a75a46b67 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> +	/* set the lane number as 0 */
> +	writel(0, dsi->regs + DSI_TXRX_CTRL);

So set lane num to 0 means pull down mipi?

BRs,
Rex

>  
>  	mtk_dsi_disable(dsi);
>  


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

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

Hello Xinlei,

On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> In the dsi_enable function, mtk_dsi_rxtx_control is to
> pull up the MIPI signal operation. Before dsi_disable,
> MIPI should also be pulled down by writing a register instead of
> disabling dsi.
> 

What will happen if you do not pulled down the mipi before disable dsi?
What's differnet for this two setting?

> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index b509d59235e2..1c6a75a46b67 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
> +	/* set the lane number as 0 */
> +	writel(0, dsi->regs + DSI_TXRX_CTRL);

So set lane num to 0 means pull down mipi?

BRs,
Rex

>  
>  	mtk_dsi_disable(dsi);
>  

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > pull up the MIPI signal operation. Before dsi_disable,
> > MIPI should also be pulled down by writing a register instead of
> > disabling dsi.
> > 
> 
> What will happen if you do not pulled down the mipi before disable
> dsi?
> What's differnet for this two setting?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index b509d59235e2..1c6a75a46b67 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > +	/* set the lane number as 0 */
> > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> 
> So set lane num to 0 means pull down mipi?
> 
> BRs,
> Rex
> 
> >  
> >  	mtk_dsi_disable(dsi);
> >  
> 
> 

Hi rex:

1. 
If you disable dsi without pulling the mipi signal low, the value of
the register will still maintain the setting of the mipi signal being
pulled high. 
After resume, even if the mipi signal is not pulled high, it will still
be in the high state.

2.So set lane num to 0 means pull down mipi
=> yes

Do you have any suggestions on the next version?

Best Regards!
xinlei
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > pull up the MIPI signal operation. Before dsi_disable,
> > MIPI should also be pulled down by writing a register instead of
> > disabling dsi.
> > 
> 
> What will happen if you do not pulled down the mipi before disable
> dsi?
> What's differnet for this two setting?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index b509d59235e2..1c6a75a46b67 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > +	/* set the lane number as 0 */
> > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> 
> So set lane num to 0 means pull down mipi?
> 
> BRs,
> Rex
> 
> >  
> >  	mtk_dsi_disable(dsi);
> >  
> 
> 

Hi rex:

1. 
If you disable dsi without pulling the mipi signal low, the value of
the register will still maintain the setting of the mipi signal being
pulled high. 
After resume, even if the mipi signal is not pulled high, it will still
be in the high state.

2.So set lane num to 0 means pull down mipi
=> yes

Do you have any suggestions on the next version?

Best Regards!
xinlei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[xinlei.lee]

On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> Hello Xinlei,
> 
> On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > pull up the MIPI signal operation. Before dsi_disable,
> > MIPI should also be pulled down by writing a register instead of
> > disabling dsi.
> > 
> 
> What will happen if you do not pulled down the mipi before disable
> dsi?
> What's differnet for this two setting?
> 
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index b509d59235e2..1c6a75a46b67 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > *dsi)
> >  	mtk_dsi_reset_engine(dsi);
> >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > +	/* set the lane number as 0 */
> > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> 
> So set lane num to 0 means pull down mipi?
> 
> BRs,
> Rex
> 
> >  
> >  	mtk_dsi_disable(dsi);
> >  
> 
> 

Hi rex:

1. 
If you disable dsi without pulling the mipi signal low, the value of
the register will still maintain the setting of the mipi signal being
pulled high. 
After resume, even if the mipi signal is not pulled high, it will still
be in the high state.

2.So set lane num to 0 means pull down mipi
=> yes

Do you have any suggestions on the next version?

Best Regards!
xinlei

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

On Tue, 2022-03-22 at 18:00 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > > pull up the MIPI signal operation. Before dsi_disable,
> > > MIPI should also be pulled down by writing a register instead of
> > > disabling dsi.
> > > 
> > 
> > What will happen if you do not pulled down the mipi before disable
> > dsi?
> > What's differnet for this two setting?
> > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index b509d59235e2..1c6a75a46b67 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > +	/* set the lane number as 0 */

Hello Xinlei,

I think you can write this comment more detailed, like
"/* set the lane number as 0 to pull down mipi */"

> > > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> > 
> > So set lane num to 0 means pull down mipi?
> > 
> > BRs,
> > Rex
> > 
> > >  
> > >  	mtk_dsi_disable(dsi);
> > >  
> > 
> > 
> 
> Hi rex:
> 
> 1. 
> If you disable dsi without pulling the mipi signal low, the value of
> the register will still maintain the setting of the mipi signal being
> pulled high. 
> After resume, even if the mipi signal is not pulled high, it will
> still
> be in the high state.
> 

I think you can describe this in commit message


BRs,
Rex

> 2.So set lane num to 0 means pull down mipi
> => yes
> 
> Do you have any suggestions on the next version?
> 
> Best Regards!
> xinlei
> 


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

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

On Tue, 2022-03-22 at 18:00 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > > pull up the MIPI signal operation. Before dsi_disable,
> > > MIPI should also be pulled down by writing a register instead of
> > > disabling dsi.
> > > 
> > 
> > What will happen if you do not pulled down the mipi before disable
> > dsi?
> > What's differnet for this two setting?
> > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index b509d59235e2..1c6a75a46b67 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > +	/* set the lane number as 0 */

Hello Xinlei,

I think you can write this comment more detailed, like
"/* set the lane number as 0 to pull down mipi */"

> > > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> > 
> > So set lane num to 0 means pull down mipi?
> > 
> > BRs,
> > Rex
> > 
> > >  
> > >  	mtk_dsi_disable(dsi);
> > >  
> > 
> > 
> 
> Hi rex:
> 
> 1. 
> If you disable dsi without pulling the mipi signal low, the value of
> the register will still maintain the setting of the mipi signal being
> pulled high. 
> After resume, even if the mipi signal is not pulled high, it will
> still
> be in the high state.
> 

I think you can describe this in commit message


BRs,
Rex

> 2.So set lane num to 0 means pull down mipi
> => yes
> 
> Do you have any suggestions on the next version?
> 
> Best Regards!
> xinlei
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3,4/4] drm/mediatek: Add pull-down MIPI operation in mtk_dsi_poweroff function

[Rex-BC Chen]

On Tue, 2022-03-22 at 18:00 +0800, xinlei.lee wrote:
> On Thu, 2022-03-17 at 20:20 +0800, Rex-BC Chen wrote:
> > Hello Xinlei,
> > 
> > On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
> > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > In the dsi_enable function, mtk_dsi_rxtx_control is to
> > > pull up the MIPI signal operation. Before dsi_disable,
> > > MIPI should also be pulled down by writing a register instead of
> > > disabling dsi.
> > > 
> > 
> > What will happen if you do not pulled down the mipi before disable
> > dsi?
> > What's differnet for this two setting?
> > 
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_dsi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index b509d59235e2..1c6a75a46b67 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -676,6 +676,8 @@ static void mtk_dsi_poweroff(struct mtk_dsi
> > > *dsi)
> > >  	mtk_dsi_reset_engine(dsi);
> > >  	mtk_dsi_lane0_ulp_mode_enter(dsi);
> > >  	mtk_dsi_clk_ulp_mode_enter(dsi);
> > > +	/* set the lane number as 0 */

Hello Xinlei,

I think you can write this comment more detailed, like
"/* set the lane number as 0 to pull down mipi */"

> > > +	writel(0, dsi->regs + DSI_TXRX_CTRL);
> > 
> > So set lane num to 0 means pull down mipi?
> > 
> > BRs,
> > Rex
> > 
> > >  
> > >  	mtk_dsi_disable(dsi);
> > >  
> > 
> > 
> 
> Hi rex:
> 
> 1. 
> If you disable dsi without pulling the mipi signal low, the value of
> the register will still maintain the setting of the mipi signal being
> pulled high. 
> After resume, even if the mipi signal is not pulled high, it will
> still
> be in the high state.
> 

I think you can describe this in commit message


BRs,
Rex

> 2.So set lane num to 0 means pull down mipi
> => yes
> 
> Do you have any suggestions on the next version?
> 
> Best Regards!
> xinlei
>