On Tue, 7 Nov 2023 at 01:30, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 11/6/2023 2:11 PM, Dmitry Baryshkov wrote: > > On Mon, 6 Nov 2023 at 20:39, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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? Ah, excuse me then, I didn't notice 24 vs 20. > > >> > >> 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@xxxxxxxxxx> > >>> --- > >>> 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 }, > >>> }, > >>> }; > >>> > > > > > > -- With best wishes Dmitry