devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guillaume Ranquet <granquet@baylibre.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: chunkuang.hu@kernel.org, p.zabel@pengutronix.de,
	airlied@linux.ie, daniel@ffwll.ch, robh+dt@kernel.org,
	maarten.lankhorst@linux.intel.com, tzimmermann@suse.de,
	matthias.bgg@gmail.com, chunfeng.yun@mediatek.com, kishon@ti.com,
	vkoul@kernel.org, deller@gmx.de, ck.hu@mediatek.com,
	jitao.shi@mediatek.com, angelogioacchino.delregno@collabora.com,
	dri-devel@lists.freedesktop.org,
	linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-phy@lists.infradead.org, linux-fbdev@vger.kernel.org,
	Markus Schneider-Pargmann <msp@baylibre.com>
Subject: Re: [PATCH v8 13/19] drm/mediatek: dpi: Add dpintf support
Date: Fri, 25 Feb 2022 03:25:55 -0800	[thread overview]
Message-ID: <CABnWg9uq_b2=UsVaDD2HzBwD0ep8CWaH-EjGd1Uud29o1gQO1w@mail.gmail.com> (raw)
In-Reply-To: <20220221094420.5oqsydyjvub7d7h3@houat>

Quoting Maxime Ripard (2022-02-21 10:44:20)
> Hi
>
> (Now I remember your series ;)
Hi,

I'm not sure this is a good thing though :)

>
> On Fri, Feb 18, 2022 at 03:54:31PM +0100, Guillaume Ranquet wrote:
> > dpintf is the displayport interface hardware unit. This unit is similar
> > to dpi and can reuse most of the code.
> >
> > This patch adds support for mt8195-dpintf to this dpi driver. Main
> > differences are:
> >  - Some features/functional components are not available for dpintf
> >    which are now excluded from code execution once is_dpintf is set
> >  - dpintf can and needs to choose between different clockdividers based
> >    on the clockspeed. This is done by choosing a different clock parent.
> >  - There are two additional clocks that need to be managed. These are
> >    only set for dpintf and will be set to NULL if not supplied. The
> >    clk_* calls handle these as normal clocks then.
> >  - Some register contents differ slightly between the two components. To
> >    work around this I added register bits/masks with a DPINTF_ prefix
> >    and use them where different.
> >
> > Based on a separate driver for dpintf created by
> > Jason-JH.Lin <jason-jh.lin@mediatek.com>.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dpi.c          | 164 +++++++++++++++++---
> >  drivers/gpu/drm/mediatek/mtk_dpi_regs.h     |  38 +++++
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |   8 +
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |   1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c      |   5 +-
> >  include/linux/soc/mediatek/mtk-mmsys.h      |   2 +
> >  6 files changed, 198 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index be99399faf1bb..c5639ada868f8 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -63,6 +63,14 @@ enum mtk_dpi_out_color_format {
> >       MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL
> >  };
> >
> > +enum TVDPLL_CLK {
> > +     TVDPLL_PLL = 0,
> > +     TVDPLL_D2 = 2,
> > +     TVDPLL_D4 = 4,
> > +     TVDPLL_D8 = 8,
> > +     TVDPLL_D16 = 16,
> > +};
> > +
> >  struct mtk_dpi {
> >       struct drm_encoder encoder;
> >       struct drm_bridge bridge;
> > @@ -71,8 +79,10 @@ struct mtk_dpi {
> >       void __iomem *regs;
> >       struct device *dev;
> >       struct clk *engine_clk;
> > +     struct clk *dpi_ck_cg;
> >       struct clk *pixel_clk;
> >       struct clk *tvd_clk;
> > +     struct clk *pclk_src[5];
> >       int irq;
> >       struct drm_display_mode mode;
> >       const struct mtk_dpi_conf *conf;
> > @@ -126,6 +136,7 @@ struct mtk_dpi_conf {
> >       const u32 *output_fmts;
> >       u32 num_output_fmts;
> >       bool is_ck_de_pol;
> > +     bool is_dpintf;
> >       bool swap_input_support;
> >       // Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH (no shift)
> >       u32 dimension_mask;
> > @@ -384,6 +395,25 @@ static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi)
> >               mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN);
> >  }
> >
> > +static void mtk_dpi_matrix_sel(struct mtk_dpi *dpi, enum mtk_dpi_out_color_format format)
> > +{
> > +     u32 matrix_sel = 0;
> > +
> > +     switch (format) {
> > +     case MTK_DPI_COLOR_FORMAT_YCBCR_422:
> > +     case MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL:
> > +     case MTK_DPI_COLOR_FORMAT_YCBCR_444:
> > +     case MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL:
> > +     case MTK_DPI_COLOR_FORMAT_XV_YCC:
> > +             if (dpi->mode.hdisplay <= 720)
> > +                     matrix_sel = 0x2;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +     mtk_dpi_mask(dpi, DPI_MATRIX_SET, matrix_sel, INT_MATRIX_SEL_MASK);
> > +}
> > +
> >  static void mtk_dpi_config_color_format(struct mtk_dpi *dpi,
> >                                       enum mtk_dpi_out_color_format format)
> >  {
> > @@ -391,6 +421,7 @@ static void mtk_dpi_config_color_format(struct mtk_dpi *dpi,
> >           (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) {
> >               mtk_dpi_config_yuv422_enable(dpi, false);
> >               mtk_dpi_config_csc_enable(dpi, true);
> > +             mtk_dpi_matrix_sel(dpi, format);
> >               if (dpi->conf->swap_input_support)
> >                       mtk_dpi_config_swap_input(dpi, false);
> >               mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_BGR);
> > @@ -398,6 +429,7 @@ static void mtk_dpi_config_color_format(struct mtk_dpi *dpi,
> >                  (format == MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL)) {
> >               mtk_dpi_config_yuv422_enable(dpi, true);
> >               mtk_dpi_config_csc_enable(dpi, true);
> > +             mtk_dpi_matrix_sel(dpi, format);
> >               if (dpi->conf->swap_input_support)
> >                       mtk_dpi_config_swap_input(dpi, true);
> >               mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB);
> > @@ -438,6 +470,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi)
> >       mtk_dpi_disable(dpi);
> >       clk_disable_unprepare(dpi->pixel_clk);
> >       clk_disable_unprepare(dpi->engine_clk);
> > +     clk_disable_unprepare(dpi->dpi_ck_cg);
> > +     clk_disable_unprepare(dpi->tvd_clk);
> >  }
> >
> >  static int mtk_dpi_power_on(struct mtk_dpi *dpi)
> > @@ -447,12 +481,24 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
> >       if (++dpi->refcount != 1)
> >               return 0;
> >
> > +     ret = clk_prepare_enable(dpi->tvd_clk);
> > +     if (ret) {
> > +             dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret);
> > +             goto err_pixel;
> > +     }
> > +
> >       ret = clk_prepare_enable(dpi->engine_clk);
> >       if (ret) {
> >               dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret);
> >               goto err_refcount;
> >       }
> >
> > +     ret = clk_prepare_enable(dpi->dpi_ck_cg);
> > +     if (ret) {
> > +             dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: %d\n", ret);
> > +             goto err_ck_cg;
> > +     }
> > +
> >       ret = clk_prepare_enable(dpi->pixel_clk);
> >       if (ret) {
> >               dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
> > @@ -462,10 +508,11 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
> >       if (dpi->pinctrl && dpi->pins_dpi)
> >               pinctrl_select_state(dpi->pinctrl, dpi->pins_dpi);
> >
> > -     mtk_dpi_enable(dpi);
> >       return 0;
> >
> >  err_pixel:
> > +     clk_disable_unprepare(dpi->dpi_ck_cg);
> > +err_ck_cg:
> >       clk_disable_unprepare(dpi->engine_clk);
> >  err_refcount:
> >       dpi->refcount--;
> > @@ -497,12 +544,21 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
> >       pll_rate = clk_get_rate(dpi->tvd_clk);
> >
> >       vm.pixelclock = pll_rate / factor;
> > -     if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) ||
> > -         (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE))
> > +     if (dpi->conf->is_dpintf) {
> > +             if (factor == 1)
> > +                     clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]);
> > +             else if (factor == 2)
> > +                     clk_set_parent(dpi->pixel_clk, dpi->pclk_src[3]);
> > +             else if (factor == 4)
> > +                     clk_set_parent(dpi->pixel_clk, dpi->pclk_src[4]);
> > +             else
> > +                     clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]);
>
> You get a reference to these clocks using:
> >
> > +     dpi->pclk_src[1] = devm_clk_get(dev, "TVDPLL_D2");
> > +     dpi->pclk_src[2] = devm_clk_get(dev, "TVDPLL_D4");
> > +     dpi->pclk_src[3] = devm_clk_get(dev, "TVDPLL_D8");
> > +     dpi->pclk_src[4] = devm_clk_get(dev, "TVDPLL_D16");
> > +
>
> So if the clock isn't there, you'll get an error pointer, won't check
> it, but will dereference it.
>
> If the clock is optional, you should use clk_get_optional, otherwise you
> should check the error returned.
>
> That clock setup is weird though. I don't have any knowledge about your
> platform, but either the clock should be a single one if it has multiple
> dividers, or the pixel_clk clock should behave as a mux and just pick
> the best parent for the rate it's given.
>
> Maxime

You're right about the error management... but even worse, those clocks
were removed in an earlier version of the patch [1]
I have reintroduced that bit of code in v7 by mistake when I applied
fixes coming from mediatek engineers.

I don't know anything about dp/dpi or display in general, but the design
posted by markus in v1 [2] seemed closed to what you are suggesting.


Thx,
Guillaume.

[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20210816192523.1739365-3-msp@baylibre.com/#24384971
[2] https://patchwork.kernel.org/project/linux-mediatek/patch/20210906193529.718845-6-msp@baylibre.com/

  reply	other threads:[~2022-02-25 11:26 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-18 14:54 [PATCH v8 0/19] drm/mediatek: Add mt8195 DisplayPort driver Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 01/19] dt-bindings: mediatek,dpi: Add DP_INTF compatible Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 02/19] dt-bindings: mediatek,dp: Add Display Port binding Guillaume Ranquet
2022-02-21  2:36   ` Rob Herring
2022-02-18 14:54 ` [PATCH v8 03/19] drm/edid: Add cea_sad helpers for freq/length Guillaume Ranquet
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-25 10:35     ` Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 04/19] video/hdmi: Add audio_infoframe packing for DP Guillaume Ranquet
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-25 10:38     ` Guillaume Ranquet
2022-02-22 15:16   ` AngeloGioacchino Del Regno
2022-02-25 10:40     ` Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 05/19] drm/mediatek: dpi: move dpi limits to board config Guillaume Ranquet
2022-02-21  1:46   ` Chun-Kuang Hu
2022-02-25 10:45     ` Guillaume Ranquet
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 06/19] drm/mediatek: dpi: implement a CK/DE pol toggle in " Guillaume Ranquet
2022-02-21  2:14   ` Chun-Kuang Hu
2022-02-25 10:52     ` Guillaume Ranquet
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 07/19] drm/mediatek: dpi: implement a swap_input " Guillaume Ranquet
2022-02-21  2:32   ` Chun-Kuang Hu
2022-02-25 10:55     ` Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 08/19] drm/mediatek: dpi: move dimension mask to " Guillaume Ranquet
2022-02-21  2:40   ` Chun-Kuang Hu
2022-02-21  2:42     ` Chun-Kuang Hu
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 09/19] drm/mediatek: dpi: move dimension_mask " Guillaume Ranquet
2022-02-21  2:43   ` Chun-Kuang Hu
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 10/19] drm/mediatek: dpi: move swap_shift " Guillaume Ranquet
2022-02-21  3:24   ` Chun-Kuang Hu
2022-02-25 11:10     ` Guillaume Ranquet
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 11/19] drm/mediatek: dpi: move the yuv422_en_bit " Guillaume Ranquet
2022-02-21  3:31   ` Chun-Kuang Hu
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 12/19] drm/mediatek: dpi: move the csc_enable bit " Guillaume Ranquet
2022-02-21  3:33   ` Chun-Kuang Hu
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 13/19] drm/mediatek: dpi: Add dpintf support Guillaume Ranquet
2022-02-21  4:34   ` Chun-Kuang Hu
2022-02-21  9:44   ` Maxime Ripard
2022-02-25 11:25     ` Guillaume Ranquet [this message]
2022-02-21 14:30   ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 14/19] phy: phy-mtk-dp: Add driver for DP phy Guillaume Ranquet
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-25 11:49     ` Guillaume Ranquet
2022-02-25 12:06       ` AngeloGioacchino Del Regno
2022-02-18 14:54 ` [PATCH v8 15/19] drm/mediatek: Add mt8195 Embedded DisplayPort driver Guillaume Ranquet
2022-02-21 14:31   ` AngeloGioacchino Del Regno
2022-02-24 13:57   ` AngeloGioacchino Del Regno
2022-02-25  9:45   ` CK Hu
2022-02-25 13:04     ` Guillaume Ranquet
2022-03-04  2:42   ` CK Hu
2022-02-18 14:54 ` [PATCH v8 16/19] drm/mediatek: Add mt8195 External DisplayPort support Guillaume Ranquet
2022-02-18 14:54 ` [PATCH v8 17/19] drm/mediatek: add hpd debounce Guillaume Ranquet
2022-03-16  6:48   ` Rex-BC Chen
2022-03-18  9:03   ` Wei-Shun Chang
2022-03-21  7:24   ` Rex-BC Chen
2022-02-18 14:54 ` [PATCH v8 18/19] drm/mediatek: change the aux retries times when receiving AUX_DEFER Guillaume Ranquet
2022-03-16 13:35   ` Chen-Yu Tsai
2022-02-18 14:54 ` [PATCH v8 19/19] drm/mediatek: DP audio support for mt8195 Guillaume Ranquet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABnWg9uq_b2=UsVaDD2HzBwD0ep8CWaH-EjGd1Uud29o1gQO1w@mail.gmail.com' \
    --to=granquet@baylibre.com \
    --cc=airlied@linux.ie \
    --cc=angelogioacchino.delregno@collabora.com \
    --cc=chunfeng.yun@mediatek.com \
    --cc=chunkuang.hu@kernel.org \
    --cc=ck.hu@mediatek.com \
    --cc=daniel@ffwll.ch \
    --cc=deller@gmx.de \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jitao.shi@mediatek.com \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-phy@lists.infradead.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=matthias.bgg@gmail.com \
    --cc=maxime@cerno.tech \
    --cc=msp@baylibre.com \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=tzimmermann@suse.de \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).