| Message ID | 20221110183853.3678209-2-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: bridge: Add Samsung MIPI DSIM bridge | expand |
On Fri, Nov 11, 2022 at 2:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies > 0 = Enable and 1 = Disable. Oh I see, that's confusing... IMHO you might want to change the register macro name... (but if that's what the datasheet uses, it might not be ideal either). At the _very_ least, I'd add a comment in the code so the next person doesn't attempt to "fix" it again... BTW, are you sure DSIM_HSE_MODE is correct now? > > The logic for checking these mode flags was correct before > the MIPI_DSI*_NO_* mode flag conversion. > > Fix the MIPI_DSI*_NO_* mode flags handling. > > Fixes: 0f3b68b66a6d ("drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling > features") > Cc: Nicolas Boichat <drinkcat@chromium.org> > Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index ec673223d6b7..b5305b145ddb 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -805,15 +805,15 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi) > reg |= DSIM_AUTO_MODE; > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE) > reg |= DSIM_HSE_MODE; > - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)) > + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP) > reg |= DSIM_HFP_MODE; > - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)) > + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP) > reg |= DSIM_HBP_MODE; > - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)) > + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA) > reg |= DSIM_HSA_MODE; > } > > - if (!(dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)) > + if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET) > reg |= DSIM_EOT_DISABLE; > > switch (dsi->format) { > -- > 2.25.1 >
On Fri, Nov 11, 2022 at 6:19 AM Nicolas Boichat <drinkcat@chromium.org> wrote: > > On Fri, Nov 11, 2022 at 2:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies > > 0 = Enable and 1 = Disable. > > Oh I see, that's confusing... IMHO you might want to change the > register macro name... (but if that's what the datasheet uses, it > might not be ideal either). At the _very_ least, I'd add a comment in > the code so the next person doesn't attempt to "fix" it again... 02/14 on the same series doing the name change. https://lore.kernel.org/all/20221110183853.3678209-3-jagan@amarulasolutions.com/ > > BTW, are you sure DSIM_HSE_MODE is correct now? Yes, we have tested in imx8m platforms as well. Sébastien Szymanski initially observed this issue on the imx8m platform. Jagan.
On Fri, Nov 11, 2022 at 4:49 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Fri, Nov 11, 2022 at 6:19 AM Nicolas Boichat <drinkcat@chromium.org> wrote: > > > > On Fri, Nov 11, 2022 at 2:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies > > > 0 = Enable and 1 = Disable. > > > > Oh I see, that's confusing... IMHO you might want to change the > > register macro name... (but if that's what the datasheet uses, it > > might not be ideal either). At the _very_ least, I'd add a comment in > > the code so the next person doesn't attempt to "fix" it again... > > 02/14 on the same series doing the name change. > https://lore.kernel.org/all/20221110183853.3678209-3-jagan@amarulasolutions.com/ Oh thanks I wasn't cc'ed on that one, makes sense. You can add my reviewed tag to this one, as my HSE comment doesn't change this. Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> But please double check HSE. > > > > > BTW, are you sure DSIM_HSE_MODE is correct now? > > Yes, we have tested in imx8m platforms as well. Sébastien Szymanski > initially observed this issue on the imx8m platform. I'll repeat, are you sure about HSE specifically? You invert the polarity for HBP, HFP, and HSA, which makes sense given your patch 02/14. I'm concerned about HSE. Is the bit really a disable bit? MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you should not do `reg |= DSIM_HSE_DISABLE;`, probably. Thanks, > > Jagan.
On Fri, Nov 11, 2022 at 5:42 PM Nicolas Boichat <drinkcat@chromium.org> wrote: > > On Fri, Nov 11, 2022 at 4:49 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Fri, Nov 11, 2022 at 6:19 AM Nicolas Boichat <drinkcat@chromium.org> wrote: > > > > > > On Fri, Nov 11, 2022 at 2:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > > > HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies > > > > 0 = Enable and 1 = Disable. > > > > > > Oh I see, that's confusing... IMHO you might want to change the > > > register macro name... (but if that's what the datasheet uses, it > > > might not be ideal either). At the _very_ least, I'd add a comment in > > > the code so the next person doesn't attempt to "fix" it again... > > > > 02/14 on the same series doing the name change. > > https://lore.kernel.org/all/20221110183853.3678209-3-jagan@amarulasolutions.com/ > > Oh thanks I wasn't cc'ed on that one, makes sense. > > You can add my reviewed tag to this one, as my HSE comment doesn't change this. > > Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> > > But please double check HSE. > > > > > > > > > BTW, are you sure DSIM_HSE_MODE is correct now? > > > > Yes, we have tested in imx8m platforms as well. Sébastien Szymanski > > initially observed this issue on the imx8m platform. > > I'll repeat, are you sure about HSE specifically? You invert the > polarity for HBP, HFP, and HSA, which makes sense given your patch > 02/14. > > I'm concerned about HSE. Is the bit really a disable bit? > MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you > should not do `reg |= DSIM_HSE_DISABLE;`, probably. HSE typically enables bit logic, unlike other bits which are disabled bits. HseDisableMod: In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit transfers Hsync end packet in Vsync pulse and Vporch area (optional). 0 = Disables transfer 1 = Enables transfer HfpDisableMode: Specifies HFP disable mode. If this bit set, DSI master ignores HFP area in Video mode. 0 = Enables 1 = Disables I think the naming of 'HseDisableMod' is misleading in the datasheet, but I have used it as per the spec. Jagan.
On 11/11/22 13:12, Nicolas Boichat wrote: [...] >>> BTW, are you sure DSIM_HSE_MODE is correct now? >> >> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski >> initially observed this issue on the imx8m platform. > > I'll repeat, are you sure about HSE specifically? You invert the > polarity for HBP, HFP, and HSA, which makes sense given your patch > 02/14. > > I'm concerned about HSE. Is the bit really a disable bit? > MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you > should not do `reg |= DSIM_HSE_DISABLE;`, probably. I suspect the HSE bit is a misnomer, but its handling in the driver is correct. i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 Page 5436 23 HseDisableMode In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit transfers Hsync end packet in Vsync pulse and Vporch area (optional). 0 = Disables transfer 1 = Enables transfer In command mode, this bit is ignored.
On Sun, Nov 13, 2022 at 8:29 AM Marek Vasut <marex@denx.de> wrote: > > On 11/11/22 13:12, Nicolas Boichat wrote: > > [...] > > >>> BTW, are you sure DSIM_HSE_MODE is correct now? > >> > >> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski > >> initially observed this issue on the imx8m platform. > > > > I'll repeat, are you sure about HSE specifically? You invert the > > polarity for HBP, HFP, and HSA, which makes sense given your patch > > 02/14. > > > > I'm concerned about HSE. Is the bit really a disable bit? > > MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you > > should not do `reg |= DSIM_HSE_DISABLE;`, probably. > > I suspect the HSE bit is a misnomer, but its handling in the driver is > correct. > > i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 > Page 5436 > > 23 HseDisableMode > > In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync > start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit > transfers Hsync end packet in Vsync pulse and Vporch area (optional). > > 0 = Disables transfer > 1 = Enables transfer > > In command mode, this bit is ignored. Okay. I'd suggest adding a comment in the code, it'd be so tempting to attempt to "fix" this as the if/or pattern looks different from the others. But it's up to you all. Thanks,
On 11/14/22 02:11, Nicolas Boichat wrote: > On Sun, Nov 13, 2022 at 8:29 AM Marek Vasut <marex@denx.de> wrote: >> >> On 11/11/22 13:12, Nicolas Boichat wrote: >> >> [...] >> >>>>> BTW, are you sure DSIM_HSE_MODE is correct now? >>>> >>>> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski >>>> initially observed this issue on the imx8m platform. >>> >>> I'll repeat, are you sure about HSE specifically? You invert the >>> polarity for HBP, HFP, and HSA, which makes sense given your patch >>> 02/14. >>> >>> I'm concerned about HSE. Is the bit really a disable bit? >>> MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you >>> should not do `reg |= DSIM_HSE_DISABLE;`, probably. >> >> I suspect the HSE bit is a misnomer, but its handling in the driver is >> correct. >> >> i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 >> Page 5436 >> >> 23 HseDisableMode >> >> In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync >> start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit >> transfers Hsync end packet in Vsync pulse and Vporch area (optional). >> >> 0 = Disables transfer >> 1 = Enables transfer >> >> In command mode, this bit is ignored. > > Okay. I'd suggest adding a comment in the code, it'd be so tempting to > attempt to "fix" this as the if/or pattern looks different from the > others. > > But it's up to you all. I agree. Clearly the discrepancy is confusing and leads to mistakes.
On 14.11.22 04:16, Marek Vasut wrote: > On 11/14/22 02:11, Nicolas Boichat wrote: >> On Sun, Nov 13, 2022 at 8:29 AM Marek Vasut <marex@denx.de> wrote: >>> >>> On 11/11/22 13:12, Nicolas Boichat wrote: >>> >>> [...] >>> >>>>>> BTW, are you sure DSIM_HSE_MODE is correct now? >>>>> >>>>> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski >>>>> initially observed this issue on the imx8m platform. >>>> >>>> I'll repeat, are you sure about HSE specifically? You invert the >>>> polarity for HBP, HFP, and HSA, which makes sense given your patch >>>> 02/14. >>>> >>>> I'm concerned about HSE. Is the bit really a disable bit? >>>> MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you >>>> should not do `reg |= DSIM_HSE_DISABLE;`, probably. >>> >>> I suspect the HSE bit is a misnomer, but its handling in the driver is >>> correct. >>> >>> i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 >>> Page 5436 >>> >>> 23 HseDisableMode >>> >>> In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync >>> start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit >>> transfers Hsync end packet in Vsync pulse and Vporch area (optional). >>> >>> 0 = Disables transfer >>> 1 = Enables transfer >>> >>> In command mode, this bit is ignored. >> >> Okay. I'd suggest adding a comment in the code, it'd be so tempting to >> attempt to "fix" this as the if/or pattern looks different from the >> others. >> >> But it's up to you all. > > I agree. Clearly the discrepancy is confusing and leads to mistakes. +1 for a comment in the code that explains the misnamed bit. Otherwise: Reviewed-By: Frieder Schrempf <frieder.schrempf@kontron.de>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index ec673223d6b7..b5305b145ddb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -805,15 +805,15 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi) reg |= DSIM_AUTO_MODE; if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_HSE) reg |= DSIM_HSE_MODE; - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)) + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP) reg |= DSIM_HFP_MODE; - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)) + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP) reg |= DSIM_HBP_MODE; - if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)) + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA) reg |= DSIM_HSA_MODE; } - if (!(dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)) + if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET) reg |= DSIM_EOT_DISABLE; switch (dsi->format) {
HFP/HBP/HSA/EOT_PACKET modes in Exynos DSI host specifies 0 = Enable and 1 = Disable. The logic for checking these mode flags was correct before the MIPI_DSI*_NO_* mode flag conversion. Fix the MIPI_DSI*_NO_* mode flags handling. Fixes: 0f3b68b66a6d ("drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling features") Cc: Nicolas Boichat <drinkcat@chromium.org> Reported-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)