| Message ID | 20231009171139.2691218-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: correct clk bit for WB2 block | expand |
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. 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. > 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@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 }, > }, > }; >
On Mon, 6 Nov 2023 at 20:39, Abhinav Kumar <quic_abhinavk@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. > > 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@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 }, > > }, > > }; > >
On 11/6/2023 2:11 PM, Dmitry Baryshkov wrote: > On Mon, 6 Nov 2023 at 20:39, Abhinav Kumar <quic_abhinavk@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@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 }, >>> }, >>> }; >>> > > >
On Tue, 7 Nov 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 11/6/2023 2:11 PM, Dmitry Baryshkov wrote: > > On Mon, 6 Nov 2023 at 20:39, Abhinav Kumar <quic_abhinavk@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? 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@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 }, > >>> }, > >>> }; > >>> > > > > > >
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 }, }, };
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. 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@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)