[PATCH 1/3] drm: pl111: Fix module autoloading

[PATCH 1/3] drm: pl111: Fix module autoloading

[Rob Herring]

Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.

Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/pl111/pl111_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index aa8aa8d9e405..7f405b1374c2 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -444,6 +444,7 @@ static const struct amba_id pl111_id_table[] = {
 	},
 	{0, 0},
 };
+MODULE_DEVICE_TABLE(amba, pl111_id_table);
 
 static struct amba_driver pl111_amba_driver __maybe_unused = {
 	.drv = {
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: pl111: Fix module autoloading

[Sam Ravnborg]

Hi Rob.

On Wed, Apr 08, 2020 at 07:39:45PM -0600, Rob Herring wrote:
> Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>

Change looks straightforward.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/pl111/pl111_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index aa8aa8d9e405..7f405b1374c2 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -444,6 +444,7 @@ static const struct amba_id pl111_id_table[] = {
>  	},
>  	{0, 0},
>  };
> +MODULE_DEVICE_TABLE(amba, pl111_id_table);
>  
>  static struct amba_driver pl111_amba_driver __maybe_unused = {
>  	.drv = {
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: pl111: Fix module autoloading

[Linus Walleij]

On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:

> Add a missing MODULE_DEVICE_TABLE entry to fix module autoloading.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: pl111: Simplify vexpress init

[Rob Herring]

The init VExpress variants currently instantiates a 'muxfpga' driver for
the sole purpose of getting a regmap for it. There's no reason to
instantiate a driver and doing so just complicates things. The muxfpga
driver also isn't unregistered properly on module unload. Let's
just simplify all this this by just calling
devm_regmap_init_vexpress_config() directly.

Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
 drivers/gpu/drm/pl111/pl111_vexpress.c  | 42 -------------------------
 drivers/gpu/drm/pl111/pl111_vexpress.h  |  7 -----
 3 files changed, 4 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 09aeaffb7660..8c2551088f26 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -8,6 +8,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/regmap.h>
+#include <linux/vexpress.h>
 
 #include "pl111_versatile.h"
 #include "pl111_vexpress.h"
@@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
 
 	/* Versatile Express special handling */
-	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
+	if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
 		struct platform_device *pdev;
-
-		/* Registers a driver for the muxfpga */
-		ret = vexpress_muxfpga_init();
-		if (ret) {
-			dev_err(dev, "unable to initialize muxfpga driver\n");
-			of_node_put(np);
-			return ret;
-		}
-
 		/* Call into deep Vexpress configuration API */
 		pdev = of_find_device_by_node(np);
 		if (!pdev) {
@@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 			of_node_put(np);
 			return -EPROBE_DEFER;
 		}
-		map = dev_get_drvdata(&pdev->dev);
-		if (!map) {
-			dev_err(dev, "sysreg has not yet probed\n");
-			platform_device_put(pdev);
-			of_node_put(np);
-			return -EPROBE_DEFER;
-		}
+		map = devm_regmap_init_vexpress_config(&pdev->dev);
+		platform_device_put(pdev);
 	} else {
 		map = syscon_node_to_regmap(np);
 	}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
index 350570fe06b5..37ed3f1da820 100644
--- a/drivers/gpu/drm/pl111/pl111_vexpress.c
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
@@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev,
 
 	return 0;
 }
-
-/*
- * This sets up the regmap pointer that will then be retrieved by
- * the detection code in pl111_versatile.c and passed in to the
- * pl111_vexpress_clcd_init() function above.
- */
-static int vexpress_muxfpga_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct regmap *map;
-
-	map = devm_regmap_init_vexpress_config(&pdev->dev);
-	if (IS_ERR(map))
-		return PTR_ERR(map);
-	dev_set_drvdata(dev, map);
-
-	return 0;
-}
-
-static const struct of_device_id vexpress_muxfpga_match[] = {
-	{ .compatible = "arm,vexpress-muxfpga", },
-	{}
-};
-
-static struct platform_driver vexpress_muxfpga_driver = {
-	.driver = {
-		.name = "vexpress-muxfpga",
-		.of_match_table = of_match_ptr(vexpress_muxfpga_match),
-	},
-	.probe = vexpress_muxfpga_probe,
-};
-
-int vexpress_muxfpga_init(void)
-{
-	int ret;
-
-	ret = platform_driver_register(&vexpress_muxfpga_driver);
-	/* -EBUSY just means this driver is already registered */
-	if (ret == -EBUSY)
-		ret = 0;
-	return ret;
-}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
index 5d3681bb4c00..bb54864ca91e 100644
--- a/drivers/gpu/drm/pl111/pl111_vexpress.h
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
@@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev,
 			     struct pl111_drm_dev_private *priv,
 			     struct regmap *map);
 
-int vexpress_muxfpga_init(void);
-
 #else
 
 static inline int pl111_vexpress_clcd_init(struct device *dev,
@@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev,
 	return -ENODEV;
 }
 
-static inline int vexpress_muxfpga_init(void)
-{
-	return 0;
-}
-
 #endif
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: pl111: Simplify vexpress init

[Sam Ravnborg]

Hi Rob.

On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote:
> The init VExpress variants currently instantiates a 'muxfpga' driver for
> the sole purpose of getting a regmap for it. There's no reason to
> instantiate a driver and doing so just complicates things. The muxfpga
> driver also isn't unregistered properly on module unload. Let's
> just simplify all this this by just calling
> devm_regmap_init_vexpress_config() directly.
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>

Procastinating, so I took a look at this.
Nice simplification - on nit below.
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
>  drivers/gpu/drm/pl111/pl111_vexpress.c  | 42 -------------------------
>  drivers/gpu/drm/pl111/pl111_vexpress.h  |  7 -----
>  3 files changed, 4 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 09aeaffb7660..8c2551088f26 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -8,6 +8,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/regmap.h>
> +#include <linux/vexpress.h>
>  
>  #include "pl111_versatile.h"
>  #include "pl111_vexpress.h"
> @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>  
>  	/* Versatile Express special handling */
> -	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> +	if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
>  		struct platform_device *pdev;
> -
> -		/* Registers a driver for the muxfpga */
> -		ret = vexpress_muxfpga_init();
> -		if (ret) {
> -			dev_err(dev, "unable to initialize muxfpga driver\n");
> -			of_node_put(np);
> -			return ret;
> -		}
> -
>  		/* Call into deep Vexpress configuration API */
>  		pdev = of_find_device_by_node(np);
>  		if (!pdev) {
> @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  			of_node_put(np);
>  			return -EPROBE_DEFER;
>  		}
> -		map = dev_get_drvdata(&pdev->dev);
> -		if (!map) {
> -			dev_err(dev, "sysreg has not yet probed\n");
> -			platform_device_put(pdev);
> -			of_node_put(np);
> -			return -EPROBE_DEFER;
> -		}
> +		map = devm_regmap_init_vexpress_config(&pdev->dev);
> +		platform_device_put(pdev);
>  	} else {
>  		map = syscon_node_to_regmap(np);
>  	}

On the following line there is:
	if (IS_ERR(map)) {
                dev_err(dev, "no Versatile syscon regmap\n");
                return PTR_ERR(map);
        }

The error message no longer tell if this was
devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that
caused the error.

	Sam

> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> index 350570fe06b5..37ed3f1da820 100644
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.c
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
> @@ -94,45 +94,3 @@ int pl111_vexpress_clcd_init(struct device *dev,
>  
>  	return 0;
>  }
> -
> -/*
> - * This sets up the regmap pointer that will then be retrieved by
> - * the detection code in pl111_versatile.c and passed in to the
> - * pl111_vexpress_clcd_init() function above.
> - */
> -static int vexpress_muxfpga_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct regmap *map;
> -
> -	map = devm_regmap_init_vexpress_config(&pdev->dev);
> -	if (IS_ERR(map))
> -		return PTR_ERR(map);
> -	dev_set_drvdata(dev, map);
> -
> -	return 0;
> -}
> -
> -static const struct of_device_id vexpress_muxfpga_match[] = {
> -	{ .compatible = "arm,vexpress-muxfpga", },
> -	{}
> -};
> -
> -static struct platform_driver vexpress_muxfpga_driver = {
> -	.driver = {
> -		.name = "vexpress-muxfpga",
> -		.of_match_table = of_match_ptr(vexpress_muxfpga_match),
> -	},
> -	.probe = vexpress_muxfpga_probe,
> -};
> -
> -int vexpress_muxfpga_init(void)
> -{
> -	int ret;
> -
> -	ret = platform_driver_register(&vexpress_muxfpga_driver);
> -	/* -EBUSY just means this driver is already registered */
> -	if (ret == -EBUSY)
> -		ret = 0;
> -	return ret;
> -}
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
> index 5d3681bb4c00..bb54864ca91e 100644
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.h
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
> @@ -10,8 +10,6 @@ int pl111_vexpress_clcd_init(struct device *dev,
>  			     struct pl111_drm_dev_private *priv,
>  			     struct regmap *map);
>  
> -int vexpress_muxfpga_init(void);
> -
>  #else
>  
>  static inline int pl111_vexpress_clcd_init(struct device *dev,
> @@ -21,9 +19,4 @@ static inline int pl111_vexpress_clcd_init(struct device *dev,
>  	return -ENODEV;
>  }
>  
> -static inline int vexpress_muxfpga_init(void)
> -{
> -	return 0;
> -}
> -
>  #endif
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: pl111: Simplify vexpress init

[Rob Herring]

On Thu, Apr 9, 2020 at 9:16 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Rob.
>
> On Wed, Apr 08, 2020 at 07:39:46PM -0600, Rob Herring wrote:
> > The init VExpress variants currently instantiates a 'muxfpga' driver for
> > the sole purpose of getting a regmap for it. There's no reason to
> > instantiate a driver and doing so just complicates things. The muxfpga
> > driver also isn't unregistered properly on module unload. Let's
> > just simplify all this this by just calling
> > devm_regmap_init_vexpress_config() directly.
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Procastinating, so I took a look at this.
> Nice simplification - on nit below.
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > ---
> >  drivers/gpu/drm/pl111/pl111_versatile.c | 21 +++----------
> >  drivers/gpu/drm/pl111/pl111_vexpress.c  | 42 -------------------------
> >  drivers/gpu/drm/pl111/pl111_vexpress.h  |  7 -----
> >  3 files changed, 4 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> > index 09aeaffb7660..8c2551088f26 100644
> > --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/regmap.h>
> > +#include <linux/vexpress.h>
> >
> >  #include "pl111_versatile.h"
> >  #include "pl111_vexpress.h"
> > @@ -325,17 +326,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> >       versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
> >
> >       /* Versatile Express special handling */
> > -     if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > +     if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> >               struct platform_device *pdev;
> > -
> > -             /* Registers a driver for the muxfpga */
> > -             ret = vexpress_muxfpga_init();
> > -             if (ret) {
> > -                     dev_err(dev, "unable to initialize muxfpga driver\n");
> > -                     of_node_put(np);
> > -                     return ret;
> > -             }
> > -
> >               /* Call into deep Vexpress configuration API */
> >               pdev = of_find_device_by_node(np);
> >               if (!pdev) {
> > @@ -343,13 +335,8 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> >                       of_node_put(np);
> >                       return -EPROBE_DEFER;
> >               }
> > -             map = dev_get_drvdata(&pdev->dev);
> > -             if (!map) {
> > -                     dev_err(dev, "sysreg has not yet probed\n");
> > -                     platform_device_put(pdev);
> > -                     of_node_put(np);
> > -                     return -EPROBE_DEFER;
> > -             }
> > +             map = devm_regmap_init_vexpress_config(&pdev->dev);
> > +             platform_device_put(pdev);
> >       } else {
> >               map = syscon_node_to_regmap(np);
> >       }
>
> On the following line there is:
>         if (IS_ERR(map)) {
>                 dev_err(dev, "no Versatile syscon regmap\n");
>                 return PTR_ERR(map);
>         }
>
> The error message no longer tell if this was
> devm_regmap_init_vexpress_config() or syscon_node_to_regmap() that
> caused the error.

Hopefully you'd know what platform you are on.

In any case, it's changed after patch 3.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: pl111: Simplify vexpress init

[Linus Walleij]

On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:

> The init VExpress variants currently instantiates a 'muxfpga' driver for
> the sole purpose of getting a regmap for it. There's no reason to
> instantiate a driver and doing so just complicates things. The muxfpga
> driver also isn't unregistered properly on module unload. Let's
> just simplify all this this by just calling
> devm_regmap_init_vexpress_config() directly.
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>

OK... looking at this.

> +       if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
>                 struct platform_device *pdev;
> -
> -               /* Registers a driver for the muxfpga */
> -               ret = vexpress_muxfpga_init();
> -               if (ret) {
> -                       dev_err(dev, "unable to initialize muxfpga driver\n");
> -                       of_node_put(np);
> -                       return ret;
> -               }
> -
>                 /* Call into deep Vexpress configuration API */
>                 pdev = of_find_device_by_node(np);

So this finds the platform device for compatible  "arm,vexpress-muxfpga",
ha!

> +               map = devm_regmap_init_vexpress_config(&pdev->dev);
> +               platform_device_put(pdev);

So then you can just do it like that.

Clever! Hats off for digging through my (unnecessary complex) code.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: pl111: Move VExpress setup into versatile init

[Rob Herring]

Since the VExpress setup in pl111_vexpress.c is now just a single
function call, let's move it into pl111_versatile.c and we can further
simplify pl111_versatile_init() by moving the other pieces for VExpress
into pl111_vexpress_clcd_init().

Cc: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/pl111/Makefile          |   1 -
 drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
 drivers/gpu/drm/pl111/pl111_vexpress.c  |  96 -------------------
 drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 -----
 4 files changed, 102 insertions(+), 139 deletions(-)
 delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
 delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h

diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
index 0c70f0e91d21..67d430d433e0 100644
--- a/drivers/gpu/drm/pl111/Makefile
+++ b/drivers/gpu/drm/pl111/Makefile
@@ -3,7 +3,6 @@ pl111_drm-y +=	pl111_display.o \
 		pl111_versatile.o \
 		pl111_drv.o
 
-pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
 pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
 pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
 
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 8c2551088f26..1b55f977e945 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -11,7 +11,6 @@
 #include <linux/vexpress.h>
 
 #include "pl111_versatile.h"
-#include "pl111_vexpress.h"
 #include "pl111_drm.h"
 
 static struct regmap *versatile_syscon_map;
@@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
 	.broken_clockdivider = true,
 };
 
+#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
+
+static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
+				    struct pl111_drm_dev_private *priv)
+{
+	struct platform_device *pdev;
+	struct device_node *root;
+	struct device_node *child;
+	struct device_node *ct_clcd = NULL;
+	struct regmap *map;
+	bool has_coretile_clcd = false;
+	bool has_coretile_hdlcd = false;
+	bool mux_motherboard = true;
+	u32 val;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
+		return -ENODEV;
+
+	/*
+	 * Check if we have a CLCD or HDLCD on the core tile by checking if a
+	 * CLCD or HDLCD is available in the root of the device tree.
+	 */
+	root = of_find_node_by_path("/");
+	if (!root)
+		return -EINVAL;
+
+	for_each_available_child_of_node(root, child) {
+		if (of_device_is_compatible(child, "arm,pl111")) {
+			has_coretile_clcd = true;
+			ct_clcd = child;
+			break;
+		}
+		if (of_device_is_compatible(child, "arm,hdlcd")) {
+			has_coretile_hdlcd = true;
+			of_node_put(child);
+			break;
+		}
+	}
+
+	of_node_put(root);
+
+	/*
+	 * If there is a coretile HDLCD and it has a driver,
+	 * do not mux the CLCD on the motherboard to the DVI.
+	 */
+	if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
+		mux_motherboard = false;
+
+	/*
+	 * On the Vexpress CA9 we let the CLCD on the coretile
+	 * take precedence, so also in this case do not mux the
+	 * motherboard to the DVI.
+	 */
+	if (has_coretile_clcd)
+		mux_motherboard = false;
+
+	if (mux_motherboard) {
+		dev_info(dev, "DVI muxed to motherboard CLCD\n");
+		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
+	} else if (ct_clcd == dev->of_node) {
+		dev_info(dev,
+			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
+		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
+	} else {
+		dev_info(dev, "core tile graphics present\n");
+		dev_info(dev, "this device will be deactivated\n");
+		return -ENODEV;
+	}
+
+	/* Call into deep Vexpress configuration API */
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		dev_err(dev, "can't find the sysreg device, deferring\n");
+		return -EPROBE_DEFER;
+	}
+
+	map = devm_regmap_init_vexpress_config(&pdev->dev);
+	if (IS_ERR(map)) {
+		platform_device_put(pdev);
+		return PTR_ERR(map);
+	}
+
+	ret = regmap_write(map, 0, val);
+	platform_device_put(pdev);
+	if (ret) {
+		dev_err(dev, "error setting DVI muxmode\n");
+		return -ENODEV;
+	}
+
+	priv->variant = &pl111_vexpress;
+	dev_info(dev, "initializing Versatile Express PL111\n");
+
+	return 0;
+}
+
 int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 {
 	const struct of_device_id *clcd_id;
 	enum versatile_clcd versatile_clcd_type;
 	struct device_node *np;
 	struct regmap *map;
-	int ret;
 
 	np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
 					     &clcd_id);
@@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
 
 	/* Versatile Express special handling */
-	if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
-		struct platform_device *pdev;
-		/* Call into deep Vexpress configuration API */
-		pdev = of_find_device_by_node(np);
-		if (!pdev) {
-			dev_err(dev, "can't find the sysreg device, deferring\n");
-			of_node_put(np);
-			return -EPROBE_DEFER;
-		}
-		map = devm_regmap_init_vexpress_config(&pdev->dev);
-		platform_device_put(pdev);
+	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
+		int ret = pl111_vexpress_clcd_init(dev, np, priv);
+		of_node_put(np);
+		return ret;
 	} else {
 		map = syscon_node_to_regmap(np);
 	}
@@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 		priv->variant_display_disable = pl111_realview_clcd_disable;
 		dev_info(dev, "set up callbacks for RealView PL111\n");
 		break;
-	case VEXPRESS_CLCD_V2M:
-		priv->variant = &pl111_vexpress;
-		dev_info(dev, "initializing Versatile Express PL111\n");
-		ret = pl111_vexpress_clcd_init(dev, priv, map);
-		if (ret)
-			return ret;
-		break;
 	default:
 		dev_info(dev, "unknown Versatile system controller\n");
 		break;
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
deleted file mode 100644
index 37ed3f1da820..000000000000
--- a/drivers/gpu/drm/pl111/pl111_vexpress.c
+++ /dev/null
@@ -1,96 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Versatile Express PL111 handling
- * Copyright (C) 2018 Linus Walleij
- *
- * This module binds to the "arm,vexpress-muxfpga" device on the
- * Versatile Express configuration bus and sets up which CLCD instance
- * gets muxed out on the DVI bridge.
- */
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/regmap.h>
-#include <linux/vexpress.h>
-#include <linux/platform_device.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
-#include "pl111_drm.h"
-#include "pl111_vexpress.h"
-
-#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
-#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
-#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
-
-int pl111_vexpress_clcd_init(struct device *dev,
-			     struct pl111_drm_dev_private *priv,
-			     struct regmap *map)
-{
-	struct device_node *root;
-	struct device_node *child;
-	struct device_node *ct_clcd = NULL;
-	bool has_coretile_clcd = false;
-	bool has_coretile_hdlcd = false;
-	bool mux_motherboard = true;
-	u32 val;
-	int ret;
-
-	/*
-	 * Check if we have a CLCD or HDLCD on the core tile by checking if a
-	 * CLCD or HDLCD is available in the root of the device tree.
-	 */
-	root = of_find_node_by_path("/");
-	if (!root)
-		return -EINVAL;
-
-	for_each_available_child_of_node(root, child) {
-		if (of_device_is_compatible(child, "arm,pl111")) {
-			has_coretile_clcd = true;
-			ct_clcd = child;
-			break;
-		}
-		if (of_device_is_compatible(child, "arm,hdlcd")) {
-			has_coretile_hdlcd = true;
-			of_node_put(child);
-			break;
-		}
-	}
-
-	of_node_put(root);
-
-	/*
-	 * If there is a coretile HDLCD and it has a driver,
-	 * do not mux the CLCD on the motherboard to the DVI.
-	 */
-	if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
-		mux_motherboard = false;
-
-	/*
-	 * On the Vexpress CA9 we let the CLCD on the coretile
-	 * take precedence, so also in this case do not mux the
-	 * motherboard to the DVI.
-	 */
-	if (has_coretile_clcd)
-		mux_motherboard = false;
-
-	if (mux_motherboard) {
-		dev_info(dev, "DVI muxed to motherboard CLCD\n");
-		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
-	} else if (ct_clcd == dev->of_node) {
-		dev_info(dev,
-			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
-		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
-	} else {
-		dev_info(dev, "core tile graphics present\n");
-		dev_info(dev, "this device will be deactivated\n");
-		return -ENODEV;
-	}
-
-	ret = regmap_write(map, 0, val);
-	if (ret) {
-		dev_err(dev, "error setting DVI muxmode\n");
-		return -ENODEV;
-	}
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
deleted file mode 100644
index bb54864ca91e..000000000000
--- a/drivers/gpu/drm/pl111/pl111_vexpress.h
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-struct device;
-struct pl111_drm_dev_private;
-struct regmap;
-
-#ifdef CONFIG_ARCH_VEXPRESS
-
-int pl111_vexpress_clcd_init(struct device *dev,
-			     struct pl111_drm_dev_private *priv,
-			     struct regmap *map);
-
-#else
-
-static inline int pl111_vexpress_clcd_init(struct device *dev,
-					   struct pl111_drm_dev_private *priv,
-					   struct regmap *map)
-{
-	return -ENODEV;
-}
-
-#endif
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init

[Sam Ravnborg]

Hi Rob.

On Wed, Apr 08, 2020 at 07:39:47PM -0600, Rob Herring wrote:
> Since the VExpress setup in pl111_vexpress.c is now just a single
> function call, let's move it into pl111_versatile.c and we can further
> simplify pl111_versatile_init() by moving the other pieces for VExpress
> into pl111_vexpress_clcd_init().
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Patch looks good - nits below, but anyway:

Acked-by: Sam Ravnborg <sam@ravnborg.org>


> ---
>  drivers/gpu/drm/pl111/Makefile          |   1 -
>  drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
>  drivers/gpu/drm/pl111/pl111_vexpress.c  |  96 -------------------
>  drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 -----
>  4 files changed, 102 insertions(+), 139 deletions(-)
>  delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
>  delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
> 
> diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> index 0c70f0e91d21..67d430d433e0 100644
> --- a/drivers/gpu/drm/pl111/Makefile
> +++ b/drivers/gpu/drm/pl111/Makefile
> @@ -3,7 +3,6 @@ pl111_drm-y +=	pl111_display.o \
>  		pl111_versatile.o \
>  		pl111_drv.o
>  
> -pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
>  pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
>  pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
>  
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 8c2551088f26..1b55f977e945 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -11,7 +11,6 @@
>  #include <linux/vexpress.h>
>  
>  #include "pl111_versatile.h"
> -#include "pl111_vexpress.h"
>  #include "pl111_drm.h"
>  
>  static struct regmap *versatile_syscon_map;
> @@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
>  	.broken_clockdivider = true,
>  };
>  
> +#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
> +
> +static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
> +				    struct pl111_drm_dev_private *priv)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *root;
> +	struct device_node *child;
> +	struct device_node *ct_clcd = NULL;
> +	struct regmap *map;
> +	bool has_coretile_clcd = false;
> +	bool has_coretile_hdlcd = false;
> +	bool mux_motherboard = true;
> +	u32 val;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
> +		return -ENODEV;
Thats was a simple way to get out of this.
Maybe a WARN() was in place, because should we ever hit this?

> +
> +	/*
> +	 * Check if we have a CLCD or HDLCD on the core tile by checking if a
> +	 * CLCD or HDLCD is available in the root of the device tree.
> +	 */
> +	root = of_find_node_by_path("/");
> +	if (!root)
> +		return -EINVAL;
> +
> +	for_each_available_child_of_node(root, child) {
> +		if (of_device_is_compatible(child, "arm,pl111")) {
> +			has_coretile_clcd = true;
> +			ct_clcd = child;
> +			break;
> +		}
> +		if (of_device_is_compatible(child, "arm,hdlcd")) {
> +			has_coretile_hdlcd = true;
> +			of_node_put(child);
> +			break;
> +		}
> +	}
> +
> +	of_node_put(root);
> +
> +	/*
> +	 * If there is a coretile HDLCD and it has a driver,
> +	 * do not mux the CLCD on the motherboard to the DVI.
> +	 */
> +	if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> +		mux_motherboard = false;
> +
> +	/*
> +	 * On the Vexpress CA9 we let the CLCD on the coretile
> +	 * take precedence, so also in this case do not mux the
> +	 * motherboard to the DVI.
> +	 */
> +	if (has_coretile_clcd)
> +		mux_motherboard = false;
> +
> +	if (mux_motherboard) {
> +		dev_info(dev, "DVI muxed to motherboard CLCD\n");
> +		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> +	} else if (ct_clcd == dev->of_node) {
> +		dev_info(dev,
> +			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> +		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> +	} else {
> +		dev_info(dev, "core tile graphics present\n");
> +		dev_info(dev, "this device will be deactivated\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Call into deep Vexpress configuration API */
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev) {
> +		dev_err(dev, "can't find the sysreg device, deferring\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	map = devm_regmap_init_vexpress_config(&pdev->dev);
> +	if (IS_ERR(map)) {
> +		platform_device_put(pdev);
> +		return PTR_ERR(map);
> +	}
> +
> +	ret = regmap_write(map, 0, val);
> +	platform_device_put(pdev);
> +	if (ret) {
> +		dev_err(dev, "error setting DVI muxmode\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->variant = &pl111_vexpress;
> +	dev_info(dev, "initializing Versatile Express PL111\n");
> +
> +	return 0;
> +}
> +
>  int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  {
>  	const struct of_device_id *clcd_id;
>  	enum versatile_clcd versatile_clcd_type;
>  	struct device_node *np;
>  	struct regmap *map;
> -	int ret;
>  
>  	np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
>  					     &clcd_id);
> @@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>  
>  	/* Versatile Express special handling */
> -	if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> -		struct platform_device *pdev;
> -		/* Call into deep Vexpress configuration API */
> -		pdev = of_find_device_by_node(np);
> -		if (!pdev) {
> -			dev_err(dev, "can't find the sysreg device, deferring\n");
> -			of_node_put(np);
> -			return -EPROBE_DEFER;
> -		}
> -		map = devm_regmap_init_vexpress_config(&pdev->dev);
> -		platform_device_put(pdev);
> +	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> +		int ret = pl111_vexpress_clcd_init(dev, np, priv);
> +		of_node_put(np);
> +		return ret;
It is often in error situations one return early, but in this case it is
a normal case. Maybe a small comment?


>  	} else {
>  		map = syscon_node_to_regmap(np);
>  	}
> @@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  		priv->variant_display_disable = pl111_realview_clcd_disable;
>  		dev_info(dev, "set up callbacks for RealView PL111\n");
>  		break;
> -	case VEXPRESS_CLCD_V2M:
> -		priv->variant = &pl111_vexpress;
> -		dev_info(dev, "initializing Versatile Express PL111\n");
> -		ret = pl111_vexpress_clcd_init(dev, priv, map);
> -		if (ret)
> -			return ret;
> -		break;

The switch no longer includes VEXPRESS_CLCD_V2M - because we will never
reach the switch in this case.
I guess some gcc falgs may cause a warning that not all enums are
accounted for. But then again, the default may supress it.

Anyway, I noticed so you got a comment.
I think current code is fine.

	Sam


>  	default:
>  		dev_info(dev, "unknown Versatile system controller\n");
>  		break;
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> deleted file mode 100644
> index 37ed3f1da820..000000000000
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.c
> +++ /dev/null
> @@ -1,96 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Versatile Express PL111 handling
> - * Copyright (C) 2018 Linus Walleij
> - *
> - * This module binds to the "arm,vexpress-muxfpga" device on the
> - * Versatile Express configuration bus and sets up which CLCD instance
> - * gets muxed out on the DVI bridge.
> - */
> -#include <linux/device.h>
> -#include <linux/module.h>
> -#include <linux/regmap.h>
> -#include <linux/vexpress.h>
> -#include <linux/platform_device.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_platform.h>
> -#include "pl111_drm.h"
> -#include "pl111_vexpress.h"
> -
> -#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
> -#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
> -#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
> -
> -int pl111_vexpress_clcd_init(struct device *dev,
> -			     struct pl111_drm_dev_private *priv,
> -			     struct regmap *map)
> -{
> -	struct device_node *root;
> -	struct device_node *child;
> -	struct device_node *ct_clcd = NULL;
> -	bool has_coretile_clcd = false;
> -	bool has_coretile_hdlcd = false;
> -	bool mux_motherboard = true;
> -	u32 val;
> -	int ret;
> -
> -	/*
> -	 * Check if we have a CLCD or HDLCD on the core tile by checking if a
> -	 * CLCD or HDLCD is available in the root of the device tree.
> -	 */
> -	root = of_find_node_by_path("/");
> -	if (!root)
> -		return -EINVAL;
> -
> -	for_each_available_child_of_node(root, child) {
> -		if (of_device_is_compatible(child, "arm,pl111")) {
> -			has_coretile_clcd = true;
> -			ct_clcd = child;
> -			break;
> -		}
> -		if (of_device_is_compatible(child, "arm,hdlcd")) {
> -			has_coretile_hdlcd = true;
> -			of_node_put(child);
> -			break;
> -		}
> -	}
> -
> -	of_node_put(root);
> -
> -	/*
> -	 * If there is a coretile HDLCD and it has a driver,
> -	 * do not mux the CLCD on the motherboard to the DVI.
> -	 */
> -	if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> -		mux_motherboard = false;
> -
> -	/*
> -	 * On the Vexpress CA9 we let the CLCD on the coretile
> -	 * take precedence, so also in this case do not mux the
> -	 * motherboard to the DVI.
> -	 */
> -	if (has_coretile_clcd)
> -		mux_motherboard = false;
> -
> -	if (mux_motherboard) {
> -		dev_info(dev, "DVI muxed to motherboard CLCD\n");
> -		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> -	} else if (ct_clcd == dev->of_node) {
> -		dev_info(dev,
> -			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> -		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> -	} else {
> -		dev_info(dev, "core tile graphics present\n");
> -		dev_info(dev, "this device will be deactivated\n");
> -		return -ENODEV;
> -	}
> -
> -	ret = regmap_write(map, 0, val);
> -	if (ret) {
> -		dev_err(dev, "error setting DVI muxmode\n");
> -		return -ENODEV;
> -	}
> -
> -	return 0;
> -}
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
> deleted file mode 100644
> index bb54864ca91e..000000000000
> --- a/drivers/gpu/drm/pl111/pl111_vexpress.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -struct device;
> -struct pl111_drm_dev_private;
> -struct regmap;
> -
> -#ifdef CONFIG_ARCH_VEXPRESS
> -
> -int pl111_vexpress_clcd_init(struct device *dev,
> -			     struct pl111_drm_dev_private *priv,
> -			     struct regmap *map);
> -
> -#else
> -
> -static inline int pl111_vexpress_clcd_init(struct device *dev,
> -					   struct pl111_drm_dev_private *priv,
> -					   struct regmap *map)
> -{
> -	return -ENODEV;
> -}
> -
> -#endif
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init

[Rob Herring]

On Thu, Apr 9, 2020 at 9:34 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Rob.
>
> On Wed, Apr 08, 2020 at 07:39:47PM -0600, Rob Herring wrote:
> > Since the VExpress setup in pl111_vexpress.c is now just a single
> > function call, let's move it into pl111_versatile.c and we can further
> > simplify pl111_versatile_init() by moving the other pieces for VExpress
> > into pl111_vexpress_clcd_init().
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
> Patch looks good - nits below, but anyway:
>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>
>
> > ---
> >  drivers/gpu/drm/pl111/Makefile          |   1 -
> >  drivers/gpu/drm/pl111/pl111_versatile.c | 122 ++++++++++++++++++++----
> >  drivers/gpu/drm/pl111/pl111_vexpress.c  |  96 -------------------
> >  drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 -----
> >  4 files changed, 102 insertions(+), 139 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
> >  delete mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
> >
> > diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> > index 0c70f0e91d21..67d430d433e0 100644
> > --- a/drivers/gpu/drm/pl111/Makefile
> > +++ b/drivers/gpu/drm/pl111/Makefile
> > @@ -3,7 +3,6 @@ pl111_drm-y +=        pl111_display.o \
> >               pl111_versatile.o \
> >               pl111_drv.o
> >
> > -pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
> >  pl111_drm-$(CONFIG_ARCH_NOMADIK) += pl111_nomadik.o
> >  pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
> >
> > diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> > index 8c2551088f26..1b55f977e945 100644
> > --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> > +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> > @@ -11,7 +11,6 @@
> >  #include <linux/vexpress.h>
> >
> >  #include "pl111_versatile.h"
> > -#include "pl111_vexpress.h"
> >  #include "pl111_drm.h"
> >
> >  static struct regmap *versatile_syscon_map;
> > @@ -309,13 +308,110 @@ static const struct pl111_variant_data pl111_vexpress = {
> >       .broken_clockdivider = true,
> >  };
> >
> > +#define VEXPRESS_FPGAMUX_MOTHERBOARD         0x00
> > +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1     0x01
> > +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2     0x02
> > +
> > +static int pl111_vexpress_clcd_init(struct device *dev, struct device_node *np,
> > +                                 struct pl111_drm_dev_private *priv)
> > +{
> > +     struct platform_device *pdev;
> > +     struct device_node *root;
> > +     struct device_node *child;
> > +     struct device_node *ct_clcd = NULL;
> > +     struct regmap *map;
> > +     bool has_coretile_clcd = false;
> > +     bool has_coretile_hdlcd = false;
> > +     bool mux_motherboard = true;
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (!IS_ENABLED(CONFIG_VEXPRESS_CONFIG))
> > +             return -ENODEV;
> Thats was a simple way to get out of this.
> Maybe a WARN() was in place, because should we ever hit this?

We didn't warn before if misconfigured. Adding an error print at the
caller should be enough.

> > +
> > +     /*
> > +      * Check if we have a CLCD or HDLCD on the core tile by checking if a
> > +      * CLCD or HDLCD is available in the root of the device tree.
> > +      */
> > +     root = of_find_node_by_path("/");
> > +     if (!root)
> > +             return -EINVAL;
> > +
> > +     for_each_available_child_of_node(root, child) {
> > +             if (of_device_is_compatible(child, "arm,pl111")) {
> > +                     has_coretile_clcd = true;
> > +                     ct_clcd = child;
> > +                     break;
> > +             }
> > +             if (of_device_is_compatible(child, "arm,hdlcd")) {
> > +                     has_coretile_hdlcd = true;
> > +                     of_node_put(child);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     of_node_put(root);
> > +
> > +     /*
> > +      * If there is a coretile HDLCD and it has a driver,
> > +      * do not mux the CLCD on the motherboard to the DVI.
> > +      */
> > +     if (has_coretile_hdlcd && IS_ENABLED(CONFIG_DRM_HDLCD))
> > +             mux_motherboard = false;
> > +
> > +     /*
> > +      * On the Vexpress CA9 we let the CLCD on the coretile
> > +      * take precedence, so also in this case do not mux the
> > +      * motherboard to the DVI.
> > +      */
> > +     if (has_coretile_clcd)
> > +             mux_motherboard = false;
> > +
> > +     if (mux_motherboard) {
> > +             dev_info(dev, "DVI muxed to motherboard CLCD\n");
> > +             val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> > +     } else if (ct_clcd == dev->of_node) {
> > +             dev_info(dev,
> > +                      "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> > +             val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> > +     } else {
> > +             dev_info(dev, "core tile graphics present\n");
> > +             dev_info(dev, "this device will be deactivated\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* Call into deep Vexpress configuration API */
> > +     pdev = of_find_device_by_node(np);
> > +     if (!pdev) {
> > +             dev_err(dev, "can't find the sysreg device, deferring\n");
> > +             return -EPROBE_DEFER;
> > +     }
> > +
> > +     map = devm_regmap_init_vexpress_config(&pdev->dev);
> > +     if (IS_ERR(map)) {
> > +             platform_device_put(pdev);
> > +             return PTR_ERR(map);
> > +     }
> > +
> > +     ret = regmap_write(map, 0, val);
> > +     platform_device_put(pdev);
> > +     if (ret) {
> > +             dev_err(dev, "error setting DVI muxmode\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     priv->variant = &pl111_vexpress;
> > +     dev_info(dev, "initializing Versatile Express PL111\n");
> > +
> > +     return 0;
> > +}
> > +
> >  int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> >  {
> >       const struct of_device_id *clcd_id;
> >       enum versatile_clcd versatile_clcd_type;
> >       struct device_node *np;
> >       struct regmap *map;
> > -     int ret;
> >
> >       np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
> >                                            &clcd_id);
> > @@ -326,17 +422,10 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> >       versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
> >
> >       /* Versatile Express special handling */
> > -     if (IS_ENABLED(CONFIG_VEXPRESS_CONFIG) && versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > -             struct platform_device *pdev;
> > -             /* Call into deep Vexpress configuration API */
> > -             pdev = of_find_device_by_node(np);
> > -             if (!pdev) {
> > -                     dev_err(dev, "can't find the sysreg device, deferring\n");
> > -                     of_node_put(np);
> > -                     return -EPROBE_DEFER;
> > -             }
> > -             map = devm_regmap_init_vexpress_config(&pdev->dev);
> > -             platform_device_put(pdev);
> > +     if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> > +             int ret = pl111_vexpress_clcd_init(dev, np, priv);
> > +             of_node_put(np);
> > +             return ret;
> It is often in error situations one return early, but in this case it is
> a normal case. Maybe a small comment?

I'm adding a print here to address your other comments and I think
that should be sufficient. I'll also move this up so the Vexpress
handling is before everything else.

>
>
> >       } else {

And I don't need this else.

> >               map = syscon_node_to_regmap(np);
> >       }
> > @@ -380,13 +469,6 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
> >               priv->variant_display_disable = pl111_realview_clcd_disable;
> >               dev_info(dev, "set up callbacks for RealView PL111\n");
> >               break;
> > -     case VEXPRESS_CLCD_V2M:
> > -             priv->variant = &pl111_vexpress;
> > -             dev_info(dev, "initializing Versatile Express PL111\n");
> > -             ret = pl111_vexpress_clcd_init(dev, priv, map);
> > -             if (ret)
> > -                     return ret;
> > -             break;
>
> The switch no longer includes VEXPRESS_CLCD_V2M - because we will never
> reach the switch in this case.
> I guess some gcc falgs may cause a warning that not all enums are
> accounted for. But then again, the default may supress it.

Yes, default suppresses that.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init

[Linus Walleij]

On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:

> Since the VExpress setup in pl111_vexpress.c is now just a single
> function call, let's move it into pl111_versatile.c and we can further
> simplify pl111_versatile_init() by moving the other pieces for VExpress
> into pl111_vexpress_clcd_init().
>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Rob Herring <robh@kernel.org>

Yeah that's much nicer, the other boards get a copy of the
Vexpress code but it's so little so it doesn't matter and besides
the Vexpress already had copies of the other boards init code.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init

[Rob Herring]

On Fri, Apr 17, 2020 at 3:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 9, 2020 at 3:39 AM Rob Herring <robh@kernel.org> wrote:
>
> > Since the VExpress setup in pl111_vexpress.c is now just a single
> > function call, let's move it into pl111_versatile.c and we can further
> > simplify pl111_versatile_init() by moving the other pieces for VExpress
> > into pl111_vexpress_clcd_init().
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Yeah that's much nicer, the other boards get a copy of the
> Vexpress code but it's so little so it doesn't matter and besides
> the Vexpress already had copies of the other boards init code.

It shouldn't change. The compiler should be smart enough to drop all
the code when IS_ENABLED(CONFIG_ARCH_VEXPRESS) is false.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel