Re: [PATCH 3/3] media: iris: Add support for SM8750 (VPU v3.5)

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/07/2025 14:02, Vikash Garodia wrote:
> 
> On 7/17/2025 4:24 PM, Krzysztof Kozlowski wrote:
>> On 17/07/2025 12:50, Dikshita Agarwal wrote:
>>>>>>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>>>>>>> +		ret = readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN + 4 * i,
>>>>>>>> +					 val, val & 0x400000, 2000, 20000);
>>>>>>>> +		if (ret)
>>>>>>>> +			goto disable_power;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
>>>>>>>> +				 val, val & BIT(0), 200, 2000);
>>>>>>> what are you polling here for?
>>>>>>
>>>>>>
>>>>>> This is not different than existing code. I don't understand why you are
>>>>>> commenting on something which is already there.
>>>>>
>>>>> Which code are you referring to?
>>>>
>>>> To the existing vpu33 which had Reviewed-by: Vikash Garodia
>>>> <quic_vgarodia@quicinc.com>
>>>>
>>>> You understand that everything here is the same, everything is a copy
>>>> while adding just few more things?
>>>>
>>>> My patch is not doing in this respect anything different that what you
>>>> reviewed.
>>>>
>>>
>>> It seems to have been missed in vpu33 power off sequence as well and should
>>> be fixed.
>>>
>>> Still, as mentioned earlier as well, your reference should be
>>> HPG/downstream driver of SM8750 not the previous generation (SM8650).
>>
>> Yes and partially no, because we write upstream code matching or
>> extending existing upstream driver. As you said earlier, downstream is
>> not the truth always:
>>
>> "That shouldn’t be the case. The downstream design is different, which
>> is why the driver requires the above code to move the GDSC"
>>
>> so here I built on top of SM8650 and re-iterate whatever mistakes are
>> there. The best if someone fixes VPU33 and then I rebase on top,
>> re-using fixed code as my base.
> 
> You have mixed different comments made earlier.


I did not mix. I used them here to show how pointless arguments you keep
making instead of focusing on technical aspects.

Once you say that, other place you say something else.

> 
> 1. Downstream GDSCs are still in HW_CTRL mode, while upstream GDSCs are migrated
> to HW_CTRL_TRIGGER. This does not need a fix in SM8650, but in the
> "iris_vpu35_power_on_hw" which you have added in this patch for SM8750.

No one discusses this.

> 
> 2. Register write "AON_WRAPPER_MVP_NOC_LPI_CONTROL" with 0x1 is needed on both
> SM8650 and SM8750, before polling AON_WRAPPER_MVP_NOC_LPI_STATUS in
> "iris_vpu35_power_off_hw" function.
> 
> I can soon submit a patch to fix SM8650 with the missing register write, but i

Great!

> do not see a need to wait for it to continue your development on SM8750.


I am not sure if you both understand how upstream development works. We
reduce to a reasonable minimum duplicated codes and different solutions,
so that's why my code is a copy of existing code plus new things for SM8750.

The goal of upstream is not to implement SM8750 completely different.
Please switch downstream approach to above re-usage approach.

And that's why your fix is important because I am going to copy exactly
that part of code and I should not come with different code.


Best regards,
Krzysztof