[PATCH 7/9] drm/meson: dw-hdmi: use matched data

Neil Armstrong neil.armstrong at linaro.org
Mon Aug 19 09:25:24 PDT 2024


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 at 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))




More information about the linux-amlogic mailing list