Re: [PATCH 4/7] drm/pl111: Enable PL110 variant

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wednesday, 2017-08-30 20:07:08 +0200, Linus Walleij wrote:
> We detect and enable the use of the PL110 variant, an earlier
> incarnation of PL111. The only real difference is that the
> control and interrupt enable registers have swapped place.
> The Versatile AB and Versatile PB have a variant inbetween
> PL110 and PL111, it is PL110 but they have already swapped the
> two registers so those two need a bit of special handling.
> 
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/pl111/pl111_display.c | 27 +++--------
>  drivers/gpu/drm/pl111/pl111_drm.h     | 17 +++++++
>  drivers/gpu/drm/pl111/pl111_drv.c     | 84 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 105 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index ef86ef60aed1..6447f36c243a 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -201,7 +201,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
>  		break;
>  	}
>  
> -	writel(cntl, priv->regs + CLCD_PL111_CNTL);
> +	writel(cntl, priv->regs + priv->ctrl);
>  
>  	drm_panel_enable(priv->panel);
>  
> @@ -219,7 +219,7 @@ void pl111_display_disable(struct drm_simple_display_pipe *pipe)
>  	drm_panel_disable(priv->panel);
>  
>  	/* Disable and Power Down */
> -	writel(0, priv->regs + CLCD_PL111_CNTL);
> +	writel(0, priv->regs + priv->ctrl);
>  
>  	drm_panel_unprepare(priv->panel);
>  
> @@ -259,7 +259,7 @@ int pl111_enable_vblank(struct drm_device *drm, unsigned int crtc)
>  {
>  	struct pl111_drm_dev_private *priv = drm->dev_private;
>  
> -	writel(CLCD_IRQ_NEXTBASE_UPDATE, priv->regs + CLCD_PL111_IENB);
> +	writel(CLCD_IRQ_NEXTBASE_UPDATE, priv->regs + priv->ienb);
>  
>  	return 0;
>  }
> @@ -268,7 +268,7 @@ void pl111_disable_vblank(struct drm_device *drm, unsigned int crtc)
>  {
>  	struct pl111_drm_dev_private *priv = drm->dev_private;
>  
> -	writel(0, priv->regs + CLCD_PL111_IENB);
> +	writel(0, priv->regs + priv->ienb);
>  }
>  
>  static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe,
> @@ -412,22 +412,6 @@ int pl111_display_init(struct drm_device *drm)
>  	struct device_node *endpoint;
>  	u32 tft_r0b0g0[3];
>  	int ret;
> -	static const u32 formats[] = {
> -		DRM_FORMAT_ABGR8888,
> -		DRM_FORMAT_XBGR8888,
> -		DRM_FORMAT_ARGB8888,
> -		DRM_FORMAT_XRGB8888,
> -		DRM_FORMAT_BGR565,
> -		DRM_FORMAT_RGB565,
> -		DRM_FORMAT_ABGR1555,
> -		DRM_FORMAT_XBGR1555,
> -		DRM_FORMAT_ARGB1555,
> -		DRM_FORMAT_XRGB1555,
> -		DRM_FORMAT_ABGR4444,
> -		DRM_FORMAT_XBGR4444,
> -		DRM_FORMAT_ARGB4444,
> -		DRM_FORMAT_XRGB4444,
> -	};
>  
>  	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>  	if (!endpoint)
> @@ -456,7 +440,8 @@ int pl111_display_init(struct drm_device *drm)
>  
>  	ret = drm_simple_display_pipe_init(drm, &priv->pipe,
>  					   &pl111_display_funcs,
> -					   formats, ARRAY_SIZE(formats),
> +					   priv->variant->formats,
> +					   priv->variant->nformats,
>  					   priv->connector);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
> index 8804af0f8997..b316a8a0fbc0 100644
> --- a/drivers/gpu/drm/pl111/pl111_drm.h
> +++ b/drivers/gpu/drm/pl111/pl111_drm.h
> @@ -31,6 +31,20 @@
>  
>  struct drm_minor;
>  
> +/**
> + * struct pl111_variant_data - encodes IP differences
> + * @name: the name of this variant
> + * @is_pl110: this is the early PL110 variant
> + * @formats: array of supported pixel formats on this variant
> + * @nformats: the length of the array of supported pixel formats
> + */
> +struct pl111_variant_data {
> +	const char *name;
> +	bool is_pl110;
> +	const u32 *formats;
> +	unsigned int nformats;
> +};
> +
>  struct pl111_drm_dev_private {
>  	struct drm_device *drm;
>  
> @@ -42,6 +56,8 @@ struct pl111_drm_dev_private {
>  	struct drm_fbdev_cma *fbdev;
>  
>  	void *regs;
> +	u32 ienb;
> +	u32 ctrl;
>  	/* The pixel clock (a reference to our clock divider off of CLCDCLK). */
>  	struct clk *clk;
>  	/* pl111's internal clock divider. */
> @@ -50,6 +66,7 @@ struct pl111_drm_dev_private {
>  	 * subsystem and pl111_display_enable().
>  	 */
>  	spinlock_t tim2_lock;
> +	struct pl111_variant_data *variant;
>  };
>  
>  int pl111_display_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index e66cbf202e17..f6863c0fb809 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -206,6 +206,7 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>  {
>  	struct device *dev = &amba_dev->dev;
>  	struct pl111_drm_dev_private *priv;
> +	struct pl111_variant_data *variant = id->data;
>  	struct drm_device *drm;
>  	int ret;
>  
> @@ -219,6 +220,33 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>  	amba_set_drvdata(amba_dev, drm);
>  	priv->drm = drm;
>  	drm->dev_private = priv;
> +	priv->variant = variant;
> +
> +	/*
> +	 * The PL110 and PL111 variants have two registers
> +	 * swapped: interrupt enable and control. For this reason
> +	 * we use offsets that we can change per variant.
> +	 */
> +	if (variant->is_pl110) {
> +		/*
> +		 * The ARM Versatile boards are even more special:
> +		 * their PrimeCell ID say they are PL110 but the
> +		 * control and interrupt enable registers are anyway
> +		 * swapped to the PL111 order so they are not following
> +		 * the PL110 datasheet.
> +		 */
> +		if (of_machine_is_compatible("arm,versatile-ab") ||
> +		    of_machine_is_compatible("arm,versatile-pb")) {
> +			priv->ienb = CLCD_PL111_IENB;
> +			priv->ctrl = CLCD_PL111_CNTL;
> +		} else {
> +			priv->ienb = CLCD_PL110_IENB;
> +			priv->ctrl = CLCD_PL110_CNTL;
> +		}
> +	} else {
> +		priv->ienb = CLCD_PL111_IENB;
> +		priv->ctrl = CLCD_PL111_CNTL;
> +	}
>  
>  	priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
>  	if (IS_ERR(priv->regs)) {
> @@ -227,10 +255,10 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>  	}
>  
>  	/* turn off interrupts before requesting the irq */
> -	writel(0, priv->regs + CLCD_PL111_IENB);
> +	writel(0, priv->regs + priv->ienb);
>  
>  	ret = devm_request_irq(dev, amba_dev->irq[0], pl111_irq, 0,
> -			       "pl111", priv);
> +			       variant->name, priv);
>  	if (ret != 0) {
>  		dev_err(dev, "%s failed irq %d\n", __func__, ret);
>  		return ret;
> @@ -269,10 +297,62 @@ static int pl111_amba_remove(struct amba_device *amba_dev)
>  	return 0;
>  }
>  
> +/*
> + * This variant exist in early versions like the ARM Integrator
> + * and this version lacks the 565 and 444 pixel formats.
> + */
> +static const u32 pl110_pixel_formats[] = {
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ABGR1555,
> +	DRM_FORMAT_XBGR1555,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_XRGB1555,
> +};
> +
> +struct pl111_variant_data pl110_variant = {

I think this (and `pl111_variant` below) can be `static const`, right?

> +	.name = "PL110",
> +	.is_pl110 = true,
> +	.formats = pl110_pixel_formats,
> +	.nformats = ARRAY_SIZE(pl110_pixel_formats),
> +};
> +
> +/* RealView, Versatile Express etc use this modern variant */
> +static const u32 pl111_pixel_formats[] = {
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_BGR565,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_ABGR1555,
> +	DRM_FORMAT_XBGR1555,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ABGR4444,
> +	DRM_FORMAT_XBGR4444,
> +	DRM_FORMAT_ARGB4444,
> +	DRM_FORMAT_XRGB4444,
> +};
> +
> +struct pl111_variant_data pl111_variant = {
> +	.name = "PL111",
> +	.formats = pl111_pixel_formats,
> +	.nformats = ARRAY_SIZE(pl111_pixel_formats),
> +};
> +
>  static struct amba_id pl111_id_table[] = {

Not 100% sure on this one, but I think it can also be `const`, as
amba_lookup() and pl111_amba_probe() are the only users I could find and
both take a `const struct amba_id *` (and pl111_amba_probe() doesn't
even use it).

(Just a couple drive-by comments, I don't know enough for an actual
review, sorry...)

>  	{
> +		.id = 0x00041110,
> +		.mask = 0x000fffff,
> +		.data = &pl110_variant,
> +	},
> +	{
>  		.id = 0x00041111,
>  		.mask = 0x000fffff,
> +		.data = &pl111_variant,
>  	},
>  	{0, 0},
>  };
> -- 
> 2.13.5
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux