From: Neil Armstrong <neil.armstrong@linaro.org>
To: Jerome Brunet <jbrunet@baylibre.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>
Cc: Kevin Hilman <khilman@baylibre.com>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
dri-devel@lists.freedesktop.org,
linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data
Date: Mon, 19 Aug 2024 18:25:24 +0200 [thread overview]
Message-ID: <0e6afba4-4312-49b4-bd92-23f12faf449a@linaro.org> (raw)
In-Reply-To: <20240730125023.710237-8-jbrunet@baylibre.com>
On 30/07/2024 14:50, Jerome Brunet wrote:
> Using several string comparisons with if/else if/else clauses
> is fairly inefficient and does not scale well.
Inefficient in which way ? speed ? code size ?
It doesn't scale, but AFAIK Amlogic stopped using the Synopsys DWC controller after the G12B SoCs....
>
> Use matched data to tweak the driver depending on the matched
> SoC instead.
This leads to a very overcomplicated code I'll need some time to review and understand
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 209 +++++++++++++++++---------
> 1 file changed, 139 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7c39e5c99043..ef059c5ef520 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -125,8 +125,17 @@
> #define HHI_HDMI_PHY_CNTL4 0x3b0 /* 0xec */
> #define HHI_HDMI_PHY_CNTL5 0x3b4 /* 0xed */
>
> +struct meson_dw_hdmi_speed {
> + const struct reg_sequence *regs;
> + unsigned int reg_num;
> + unsigned int limit;
> +};
> +
> struct meson_dw_hdmi_data {
> int (*reg_init)(struct device *dev);
> + const struct meson_dw_hdmi_speed *speeds;
> + unsigned int speed_num;
> + bool use_drm_infoframe;
> u32 cntl0_init;
> u32 cntl1_init;
> };
> @@ -185,78 +194,33 @@ struct meson_dw_hdmi {
> struct regmap *top;
> };
>
> -static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
> - const char *compat)
> -{
> - return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
> -}
> -
> -/* Bridge */
> -
> /* Setup PHY bandwidth modes */
> -static void meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
> +static int meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
> const struct drm_display_mode *mode,
> bool mode_is_420)
> {
> struct meson_drm *priv = dw_hdmi->priv;
> unsigned int pixel_clock = mode->clock;
> + int i;
>
> /* For 420, pixel clock is half unlike venc clock */
> - if (mode_is_420) pixel_clock /= 2;
> -
> - if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> - dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x333d3282);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2136315b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303382);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2036315b);
> - } else if (pixel_clock >= 148500) {
> - /* 1.485Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303362);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2016315b);
> - } else {
> - /* 742.5Mbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33604142);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x0016315b);
> - }
> - } else if (dw_hdmi_is_compatible(dw_hdmi,
> - "amlogic,meson-gxbb-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33353245);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2100115b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33634283);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0xb000115b);
> - } else {
> - /* 1.485Gbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33632122);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2000115b);
> - }
> - } else if (dw_hdmi_is_compatible(dw_hdmi,
> - "amlogic,meson-g12a-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x37eb65c4);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x0000080b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb6262);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> - } else {
> - /* 1.485Gbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb4242);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> - }
> + if (mode_is_420)
> + pixel_clock /= 2;
> +
> + for (i = 0; i < dw_hdmi->data->speed_num; i++) {
> + if (pixel_clock >= dw_hdmi->data->speeds[i].limit)
> + break;
> }
> +
> + /* No match found - Last entry should have a 0 limit */
> + if (WARN_ON(i == dw_hdmi->data->speed_num))
> + return -EINVAL;
> +
> + regmap_multi_reg_write(priv->hhi,
> + dw_hdmi->data->speeds[i].regs,
> + dw_hdmi->data->speeds[i].reg_num);
> +
> + return 0;
> }
>
> static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
> @@ -543,22 +507,133 @@ static int meson_dw_init_regmap_g12(struct device *dev)
> return 0;
> }
>
> +static const struct reg_sequence gxbb_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33353245 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2100115b },
> +};
> +
> +static const struct reg_sequence gxbb_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33634283 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0xb000115b },
> +};
> +
> +static const struct reg_sequence gxbb_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33632122 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2000115b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxbb_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = gxbb_3g7_regs,
> + .reg_num = ARRAY_SIZE(gxbb_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = gxbb_3g_regs,
> + .reg_num = ARRAY_SIZE(gxbb_3g_regs)
> + }, {
> + .regs = gxbb_def_regs,
> + .reg_num = ARRAY_SIZE(gxbb_def_regs)
> + }
> +};
> +
> static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
> .reg_init = meson_dw_init_regmap_gx,
> .cntl0_init = 0x0,
> .cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
> + .use_drm_infoframe = false,
> + .speeds = gxbb_speeds,
> + .speed_num = ARRAY_SIZE(gxbb_speeds),
> +};
> +
> +static const struct reg_sequence gxl_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x333d3282 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2136315b },
> +};
> +
> +static const struct reg_sequence gxl_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303382 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2036315b },
> +};
> +
> +static const struct reg_sequence gxl_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303362 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2016315b },
> +};
> +
> +static const struct reg_sequence gxl_270m_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33604142 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x0016315b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = gxl_3g7_regs,
> + .reg_num = ARRAY_SIZE(gxl_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = gxl_3g_regs,
> + .reg_num = ARRAY_SIZE(gxl_3g_regs)
> + }, {
> + .limit = 148500,
> + .regs = gxl_def_regs,
> + .reg_num = ARRAY_SIZE(gxl_def_regs)
> + }, {
> + .regs = gxl_270m_regs,
> + .reg_num = ARRAY_SIZE(gxl_270m_regs)
> + }
> };
>
> static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
> .reg_init = meson_dw_init_regmap_gx,
> .cntl0_init = 0x0,
> .cntl1_init = PHY_CNTL1_INIT,
> + .use_drm_infoframe = true,
> + .speeds = gxl_speeds,
> + .speed_num = ARRAY_SIZE(gxl_speeds),
> +};
> +
> +static const struct reg_sequence g12a_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x37eb65c4 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x0000080b },
> +};
> +
> +static const struct reg_sequence g12a_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb6262 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct reg_sequence g12a_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb4242 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct meson_dw_hdmi_speed g12a_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = g12a_3g7_regs,
> + .reg_num = ARRAY_SIZE(g12a_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = g12a_3g_regs,
> + .reg_num = ARRAY_SIZE(g12a_3g_regs)
> + }, {
> + .regs = g12a_def_regs,
> + .reg_num = ARRAY_SIZE(g12a_def_regs)
> + }
> };
>
> static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
> .reg_init = meson_dw_init_regmap_g12,
> .cntl0_init = 0x000b4242, /* Bandgap */
> .cntl1_init = PHY_CNTL1_INIT,
> + .use_drm_infoframe = true,
> + .speeds = g12a_speeds,
> + .speed_num = ARRAY_SIZE(g12a_speeds),
> };
>
> static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> @@ -590,7 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> platform_set_drvdata(pdev, meson_dw_hdmi);
>
> meson_dw_hdmi->priv = priv;
> - meson_dw_hdmi->dev = dev;
Unrelated change
> meson_dw_hdmi->data = match;
> dw_plat_data = &meson_dw_hdmi->dw_plat_data;
>
> @@ -650,7 +724,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> meson_dw_hdmi_init(meson_dw_hdmi);
>
> /* Bridge / Connector */
> -
Unrelated change
> dw_plat_data->priv_data = meson_dw_hdmi;
> dw_plat_data->phy_ops = &meson_dw_hdmi_phy_ops;
> dw_plat_data->phy_name = "meson_dw_hdmi_phy";
> @@ -659,11 +732,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> dw_plat_data->ycbcr_420_allowed = true;
> dw_plat_data->disable_cec = true;
> dw_plat_data->output_port = 1;
> -
> - if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> - dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
> - dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
> - dw_plat_data->use_drm_infoframe = true;
> + dw_plat_data->use_drm_infoframe = meson_dw_hdmi->data->use_drm_infoframe;
Move this to a separate patch
>
> meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
> if (IS_ERR(meson_dw_hdmi->hdmi))
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2024-08-19 16:25 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-30 12:50 [PATCH 0/9] drm/meson: dw-hdmi: clean-up Jerome Brunet
2024-07-30 12:50 ` [PATCH 1/9] drm/meson: hdmi: move encoder settings out of phy driver Jerome Brunet
2024-08-19 16:01 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH 2/9] drm/meson: vclk: drop hdmi system clock setup Jerome Brunet
2024-08-06 20:24 ` Martin Blumenstingl
2024-08-19 16:01 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH 3/9] drm/meson: dw-hdmi: use generic clock helpers Jerome Brunet
2024-08-06 20:28 ` Martin Blumenstingl
2024-08-07 7:59 ` Jerome Brunet
2024-08-19 16:02 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH 4/9] drm/meson: dw-hdmi: fix incorrect comment in suspend Jerome Brunet
2024-08-06 20:30 ` Martin Blumenstingl
2024-08-19 16:07 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH 5/9] drm/meson: dw-hdmi: split resets out of hw init Jerome Brunet
2024-08-06 20:49 ` Martin Blumenstingl
2024-08-07 8:26 ` Jerome Brunet
2024-08-19 16:08 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH 6/9] drm/meson: dw-hdmi: convert to regmap Jerome Brunet
2024-08-19 16:22 ` Neil Armstrong
2024-08-19 17:20 ` Jerome Brunet
2024-07-30 12:50 ` [PATCH 7/9] drm/meson: dw-hdmi: use matched data Jerome Brunet
2024-08-06 21:03 ` Martin Blumenstingl
2024-08-07 9:12 ` Jerome Brunet
2024-08-19 16:25 ` Neil Armstrong [this message]
2024-07-30 12:50 ` [PATCH LATER 8/9] drm/meson: dw-hdmi: don't write power controller registers Jerome Brunet
2024-08-19 16:27 ` Neil Armstrong
2024-07-30 12:50 ` [PATCH LATER 9/9] drm/meson: dw-hdmi: drop hdmi system clock setup Jerome Brunet
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=0e6afba4-4312-49b4-bd92-23f12faf449a@linaro.org \
--to=neil.armstrong@linaro.org \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=jbrunet@baylibre.com \
--cc=khilman@baylibre.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=martin.blumenstingl@googlemail.com \
--cc=mripard@kernel.org \
--cc=tzimmermann@suse.de \
/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
all inboxes | Powered by JetHome®