[v2,02/18] media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts

Message ID 20230228-topic-venus-v2-2-d95d14949c79@linaro.org
State New
Headers
Series Venus QoL / maintainability fixes |

Commit Message

Konrad Dybcio May 4, 2023, 8 a.m. UTC
  The downstream driver signals the hardware to be enabled only after the
interrupts are unmasked, which... makes sense. Follow suit.

Cc: stable@vger.kernel.org # v4.12+
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Vikash Garodia May 5, 2023, 12:33 p.m. UTC | #1
On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
> The downstream driver signals the hardware to be enabled only after the
> interrupts are unmasked, which... makes sense. Follow suit.

Rephrase the commit text,

1. No need to mention downstream driver, if something is buggy, fix it.

2. Avoid "..." and lets make it more formal.

> Cc: stable@vger.kernel.org # v4.12+
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index bff435abd59b..8fc8f46dc390 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>   	void __iomem *wrapper_base = hdev->core->wrapper_base;
>   	int ret = 0;
>   
> -	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>   	if (IS_V6(hdev->core)) {
>   		mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>   		mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>   	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>   	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>   
> +	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>   	while (!ctrl_status && count < max_tries) {
>   		ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>   		if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {

Above code looks good.

-Vikash
  
Konrad Dybcio May 5, 2023, 6:48 p.m. UTC | #2
On 5.05.2023 14:33, Vikash Garodia wrote:
> 
> On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
>> The downstream driver signals the hardware to be enabled only after the
>> interrupts are unmasked, which... makes sense. Follow suit.
> 
> Rephrase the commit text,
> 
> 1. No need to mention downstream driver, if something is buggy, fix it.
Generally I'd agree, however in this specific case the downstream
driver is the only available source of knowledge about what the correct
(or at least working) initialization sequence of this hw block is.

> 
> 2. Avoid "..." and lets make it more formal.
Ack

Konrad
> 
>> Cc: stable@vger.kernel.org # v4.12+
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index bff435abd59b..8fc8f46dc390 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>       void __iomem *wrapper_base = hdev->core->wrapper_base;
>>       int ret = 0;
>>   -    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>       if (IS_V6(hdev->core)) {
>>           mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>>           mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
>> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>       writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>>       writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>>   +    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>       while (!ctrl_status && count < max_tries) {
>>           ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>>           if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {
> 
> Above code looks good.
> 
> -Vikash
>
  
Vikash Garodia May 9, 2023, 11:42 a.m. UTC | #3
On 5/6/2023 12:18 AM, Konrad Dybcio wrote:
> 
> 
> On 5.05.2023 14:33, Vikash Garodia wrote:
>>
>> On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
>>> The downstream driver signals the hardware to be enabled only after the
>>> interrupts are unmasked, which... makes sense. Follow suit.
>>
>> Rephrase the commit text,
>>
>> 1. No need to mention downstream driver, if something is buggy, fix it.
> Generally I'd agree, however in this specific case the downstream
> driver is the only available source of knowledge about what the correct
> (or at least working) initialization sequence of this hw block is.
Ideal way would be to refer the sequence of register programming from hardware
programming guide (HPG). In this case, the commit can describe the change which
is being done and reviewer/maintainer can certify the sequence from the HPG.
Also downstream driver can be buggy too, so IMO, it is not the right reference.

-Vikash

>>
>> 2. Avoid "..." and lets make it more formal.
> Ack
> 
> Konrad
>>
>>> Cc: stable@vger.kernel.org # v4.12+
>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> index bff435abd59b..8fc8f46dc390 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>>       void __iomem *wrapper_base = hdev->core->wrapper_base;
>>>       int ret = 0;
>>>   -    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>>       if (IS_V6(hdev->core)) {
>>>           mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>>>           mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
>>> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>>       writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>>>       writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>>>   +    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>>       while (!ctrl_status && count < max_tries) {
>>>           ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>>>           if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {
>>
>> Above code looks good.
>>
>> -Vikash
>>
  

Patch

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index bff435abd59b..8fc8f46dc390 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -453,7 +453,6 @@  static int venus_boot_core(struct venus_hfi_device *hdev)
 	void __iomem *wrapper_base = hdev->core->wrapper_base;
 	int ret = 0;
 
-	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
 	if (IS_V6(hdev->core)) {
 		mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
 		mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
@@ -464,6 +463,7 @@  static int venus_boot_core(struct venus_hfi_device *hdev)
 	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
 	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
 
+	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
 	while (!ctrl_status && count < max_tries) {
 		ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
 		if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {