[Freedreno] [PATCH] drm/msm/dpu: correct clk bit for WB2 block

[Abhinav Kumar]

On 11/6/2023 2:11 PM, Dmitry Baryshkov wrote:
> On Mon, 6 Nov 2023 at 20:39, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>> Sorry for the delay in getting back on this. There was quite a bit of
>> history digging I had to do myself to give a certain response.
>>
>>
>> On 10/9/2023 10:11 AM, Dmitry Baryshkov wrote:
>>> On sc7280 there are two clk bits for WB2: control and status. While
>>> programming the VBIF params of WB, the driver should be toggling the
>>> former bit, while the sc7280_mdp struct lists the latter one.
>>>
>>
>> No, this is not correct. Both are control bits. But for the context of
>> where this is being used today, that is setting the VBIF OT limit, we
>> should be using the VBIF_CLI one. So the below change itself is correct
>> but not the commit text.
> 
> Maybe you can update dt bindings for the SDE driver? Because they
> clearly speak about the control and status bits.
> 

There is nothing to update here if we both are referring to the same 
entries in the dt bindings.

qcom,sde-wb-clk-status = <0x3bc 20>;

the clk status is indeed bit 20 of 0x3bc.

What we have before your patch was bit 24 of 0x3b8 which was indeed 
clk_ctl bit for wb2. But the only issue was it was not the vbif_cli one.

So we are talking about two different registers?

>>
>> We need to make the same change on sm8250 WB2 as well as this register
>> is present there too. In fact, anything >=msm8994 for setting VBIF OT
>> for WB2 we should be using VBIF_CLI bits of register MDP_CLK_CTRL2
>> (offset 0x2bc).
>>
>> For anything >=sm8550, we need to use WB_2_CLK_CTRL present within the
>> WB block and not the one in the top.
>>
>> Hence for this change, we can do below:
>>
>> -> update the commit text to indicate both are control bits but for the
>> vbif ot context we should using the corrected one
>> -> if you can also add sm8250 in the same change i can ack it and pick it up
>>
>> Have you re-validated WB with this change? If not, let me know I shall
>> while picking this up for -fixes.
> 
> No, I haven't validated this on sc7280. I'll try this on sm8250 and
> then I'll send v2.
> 
>>
>>> Correct that to ensure proper programming sequence for WB2 on sc7280.
>>>
>>> Fixes: 3ce166380567 ("drm/msm/dpu: add writeback support for sc7280")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> index 3b5061c4402a..9195cb996f44 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> @@ -25,7 +25,7 @@ static const struct dpu_mdp_cfg sc7280_mdp = {
>>>                [DPU_CLK_CTRL_DMA0] = { .reg_off = 0x2ac, .bit_off = 8 },
>>>                [DPU_CLK_CTRL_DMA1] = { .reg_off = 0x2b4, .bit_off = 8 },
>>>                [DPU_CLK_CTRL_DMA2] = { .reg_off = 0x2c4, .bit_off = 8 },
>>> -             [DPU_CLK_CTRL_WB2] = { .reg_off = 0x3b8, .bit_off = 24 },
>>> +             [DPU_CLK_CTRL_WB2] = { .reg_off = 0x2bc, .bit_off = 16 },
>>>        },
>>>    };
>>>
> 
> 
>