From: Rob Herring <robh@kernel.org>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Eric Anholt <eric@anholt.net>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>,
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init
Date: Fri, 17 Apr 2020 12:33:29 -0500 [thread overview]
Message-ID: <CAL_Jsq+pKubwZmmxr+EFVH_goDZ7zgeL912ChKsXS0P_tG-7Kg@mail.gmail.com> (raw)
In-Reply-To: <20200409143448.GC4673@ravnborg.org>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-04-17 17:33 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-09 1:39 [PATCH 0/3] drm: pl111: Fix module loading issues Rob Herring
2020-04-09 1:39 ` [PATCH 1/3] drm: pl111: Fix module autoloading Rob Herring
2020-04-09 14:12 ` Sam Ravnborg
2020-04-17 20:09 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 2/3] drm: pl111: Simplify vexpress init Rob Herring
2020-04-09 14:16 ` Sam Ravnborg
2020-04-17 17:43 ` Rob Herring
2020-04-17 20:23 ` Linus Walleij
2020-04-09 1:39 ` [PATCH 3/3] drm: pl111: Move VExpress setup into versatile init Rob Herring
2020-04-09 14:34 ` Sam Ravnborg
2020-04-17 17:33 ` Rob Herring [this message]
2020-04-17 20:27 ` Linus Walleij
2020-04-19 17:12 ` Rob Herring
2020-04-17 20:28 ` [PATCH 0/3] drm: pl111: Fix module loading issues Linus Walleij
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=CAL_Jsq+pKubwZmmxr+EFVH_goDZ7zgeL912ChKsXS0P_tG-7Kg@mail.gmail.com \
--to=robh@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=eric@anholt.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=sam@ravnborg.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).