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
