drm/rockchip: vop: do axi reset in vop initial time
diff mbox

Message ID 1464772752-9808-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang June 1, 2016, 9:19 a.m. UTC
There is a bug in RK3399 VOP, when bootloader/kernel only enable
VOP Big or VOP Little to display, then VOP IOMMU would failed to
reset at the initial time and VOP register couldn't write rightly.

After do the pure reset of VOP module, then things back to right.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

yao mark June 1, 2016, 9:49 a.m. UTC | #1
On 2016年06月01日 17:19, Yakir Yang wrote:
> There is a bug in RK3399 VOP, when bootloader/kernel only enable
> VOP Big or VOP Little to display, then VOP IOMMU would failed to
> reset at the initial time and VOP register couldn't write rightly.
>
> After do the pure reset of VOP module, then things back to right.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Hi Yakir

I think you get vop iommu reset failed should be IOMMU Page fault, the 
vop iommu access a ummap address.

You should ensure all vop windows access right iommu mapping address 
before attach iommu.

Actually, I'm working no flash display from bootloader to kernel, and 
prepare remove all the reset on vop initial, because it would flash the 
display.

Thanks.

> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 1c4d5b5..4150323 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1292,7 +1292,7 @@ static int vop_initial(struct vop *vop)
>   {
>   	const struct vop_data *vop_data = vop->data;
>   	const struct vop_reg_data *init_table = vop_data->init_table;
> -	struct reset_control *ahb_rst;
> +	struct reset_control *ahb_rst, *axi_rst;
>   	int i, ret;
>   
>   	vop->hclk = devm_clk_get(vop->dev, "hclk_vop");
> @@ -1331,6 +1331,19 @@ static int vop_initial(struct vop *vop)
>   	}
>   
>   	/*
> +	 * do aclk_reset, reset all vop registers.
> +	 */
> +	axi_rst = devm_reset_control_get(vop->dev, "axi");
> +	if (IS_ERR(axi_rst)) {
> +		dev_err(vop->dev, "failed to get axi reset\n");
> +		ret = PTR_ERR(axi_rst);
> +		goto err_disable_aclk;
> +	}
> +	reset_control_assert(axi_rst);
> +	usleep_range(10, 20);
> +	reset_control_deassert(axi_rst);
> +
> +	/*
>   	 * do hclk_reset, reset all vop registers.
>   	 */
>   	ahb_rst = devm_reset_control_get(vop->dev, "ahb");
Thierry Reding June 1, 2016, 3:19 p.m. UTC | #2
On Wed, Jun 01, 2016 at 05:19:12PM +0800, Yakir Yang wrote:
> There is a bug in RK3399 VOP, when bootloader/kernel only enable
> VOP Big or VOP Little to display, then VOP IOMMU would failed to
> reset at the initial time and VOP register couldn't write rightly.
> 
> After do the pure reset of VOP module, then things back to right.
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 1c4d5b5..4150323 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1292,7 +1292,7 @@ static int vop_initial(struct vop *vop)
>  {
>  	const struct vop_data *vop_data = vop->data;
>  	const struct vop_reg_data *init_table = vop_data->init_table;
> -	struct reset_control *ahb_rst;
> +	struct reset_control *ahb_rst, *axi_rst;
>  	int i, ret;
>  
>  	vop->hclk = devm_clk_get(vop->dev, "hclk_vop");
> @@ -1331,6 +1331,19 @@ static int vop_initial(struct vop *vop)
>  	}
>  
>  	/*
> +	 * do aclk_reset, reset all vop registers.
> +	 */
> +	axi_rst = devm_reset_control_get(vop->dev, "axi");

I don't think you want the managed version here since you only store
this in a local variable and hence aren't going to need it afterwards.
Just use reset_control_get() and ...

> +	if (IS_ERR(axi_rst)) {
> +		dev_err(vop->dev, "failed to get axi reset\n");
> +		ret = PTR_ERR(axi_rst);
> +		goto err_disable_aclk;
> +	}
> +	reset_control_assert(axi_rst);
> +	usleep_range(10, 20);
> +	reset_control_deassert(axi_rst);

call reset_control_put() here.

> +
> +	/*
>  	 * do hclk_reset, reset all vop registers.
>  	 */
>  	ahb_rst = devm_reset_control_get(vop->dev, "ahb");

This uses the same pattern, so you might want to consider reworking this
as well, though it should be a separate patch.

Thierry
Yakir Yang June 2, 2016, 3:11 a.m. UTC | #3
On 06/01/2016 11:19 PM, Thierry Reding wrote:
> On Wed, Jun 01, 2016 at 05:19:12PM +0800, Yakir Yang wrote:
>> There is a bug in RK3399 VOP, when bootloader/kernel only enable
>> VOP Big or VOP Little to display, then VOP IOMMU would failed to
>> reset at the initial time and VOP register couldn't write rightly.
>>
>> After do the pure reset of VOP module, then things back to right.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 1c4d5b5..4150323 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -1292,7 +1292,7 @@ static int vop_initial(struct vop *vop)
>>   {
>>   	const struct vop_data *vop_data = vop->data;
>>   	const struct vop_reg_data *init_table = vop_data->init_table;
>> -	struct reset_control *ahb_rst;
>> +	struct reset_control *ahb_rst, *axi_rst;
>>   	int i, ret;
>>   
>>   	vop->hclk = devm_clk_get(vop->dev, "hclk_vop");
>> @@ -1331,6 +1331,19 @@ static int vop_initial(struct vop *vop)
>>   	}
>>   
>>   	/*
>> +	 * do aclk_reset, reset all vop registers.
>> +	 */
>> +	axi_rst = devm_reset_control_get(vop->dev, "axi");
> I don't think you want the managed version here since you only store
> this in a local variable and hence aren't going to need it afterwards.
> Just use reset_control_get() and ...

Great, but seems Mark would not accept this fix now :(
Maybe I should found the root cause why my bootloader/kernel would 
broken the VOP.

Thanks,
- Yakir
>> +	if (IS_ERR(axi_rst)) {
>> +		dev_err(vop->dev, "failed to get axi reset\n");
>> +		ret = PTR_ERR(axi_rst);
>> +		goto err_disable_aclk;
>> +	}
>> +	reset_control_assert(axi_rst);
>> +	usleep_range(10, 20);
>> +	reset_control_deassert(axi_rst);
> call reset_control_put() here.
>
>> +
>> +	/*
>>   	 * do hclk_reset, reset all vop registers.
>>   	 */
>>   	ahb_rst = devm_reset_control_get(vop->dev, "ahb");
> This uses the same pattern, so you might want to consider reworking this
> as well, though it should be a separate patch.
>
> Thierry

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 1c4d5b5..4150323 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1292,7 +1292,7 @@  static int vop_initial(struct vop *vop)
 {
 	const struct vop_data *vop_data = vop->data;
 	const struct vop_reg_data *init_table = vop_data->init_table;
-	struct reset_control *ahb_rst;
+	struct reset_control *ahb_rst, *axi_rst;
 	int i, ret;
 
 	vop->hclk = devm_clk_get(vop->dev, "hclk_vop");
@@ -1331,6 +1331,19 @@  static int vop_initial(struct vop *vop)
 	}
 
 	/*
+	 * do aclk_reset, reset all vop registers.
+	 */
+	axi_rst = devm_reset_control_get(vop->dev, "axi");
+	if (IS_ERR(axi_rst)) {
+		dev_err(vop->dev, "failed to get axi reset\n");
+		ret = PTR_ERR(axi_rst);
+		goto err_disable_aclk;
+	}
+	reset_control_assert(axi_rst);
+	usleep_range(10, 20);
+	reset_control_deassert(axi_rst);
+
+	/*
 	 * do hclk_reset, reset all vop registers.
 	 */
 	ahb_rst = devm_reset_control_get(vop->dev, "ahb");