[PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms

Hi Arnd,

Could you comment on these?
Thanks.


Best regards,
Yangbo Lu

-----Original Message-----
From: Scott Wood [mailto:oss at buserror.net]
Sent: Saturday, June 11, 2016 9:51 AM
To: Arnd Bergmann; linuxppc-dev at lists.ozlabs.org
Cc: Mark Rutland; Ulf Hansson; linux-kernel at vger.kernel.org; linux-
i2c at vger.kernel.org; linux-clk at vger.kernel.org; Qiang Zhao; Russell King;
Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree at vger.kernel.org;
Kumar Gala; Rob Herring; Santosh Shilimkar; linux-arm-
kernel at lists.infradead.org; netdev at vger.kernel.org; linux-
mmc at vger.kernel.org; Xiaobo Xie; Yang-Leo Li; iommu at lists.linux-
foundation.org; Yangbo Lu
Subject: Re: [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms

On Thu, 2016-06-02 at 10:43 +0200, Arnd Bergmann wrote:

quoted

On Wednesday, June 1, 2016 8:47:22 PM CEST Scott Wood wrote:

quoted

On Mon, 2016-05-30 at 15:15 +0200, Arnd Bergmann wrote:

quoted

diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c new

file mode 100644 index 000000000000..2f30698f5bcf

--- /dev/null
+++ b/drivers/soc/fsl/guts.c

@@ -0,0 +1,130 @@
+/*
+ * Freescale QorIQ Platforms GUTS Driver
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License as
+published by
+ * the Free Software Foundation; either version 2 of the License,
+or
+ * (at your option) any later version.
+ */
+
+#include <linux/io.h>
+#include <linux/platform_device.h> #include <linux/module.h>
+#include <linux/slab.h> #include <linux/of_address.h> #include
+<linux/of_platform.h> #include <linux/sys_soc.h>
+
+#define GUTS_PVR	0x0a0
+#define GUTS_SVR	0x0a4
+
+struct guts {
+	void __iomem *regs;

We already have a struct to define guts.  Why are you not using it?
Why do you consider using it to be "abuse"?  What if we want to move
more guts functionality into this driver?

This structure was in the original patch, I left it in there, only
removed the inclusion of the powerpc header file, which seemed to be
misplaced.

I'm not refering "struct guts".  I'm referring to changing "struct
ccsr_guts __iomem *regs" into "void __iomem *regs".

And it's not a powerpc header file.

quoted

quoted

quoted

+/*
+ * Table for matching compatible strings, for device tree
+ * guts node, for Freescale QorIQ SOCs.
+ */
+static const struct of_device_id fsl_guts_of_match[] = {
+	/* For T4 & B4 Series SOCs */
+	{ .compatible = "fsl,qoriq-device-config-1.0", .data = "T4/B4
series" },

[snip]

quoted

+	{ .compatible = "fsl,qoriq-device-config-2.0", .data = "P
series"

As noted in my comment on patch 3/4, these descriptions are reversed.

They're also incomplete.  t2080 has device config 2.0.  t1040 is
described as
2.0 though it should probably be 2.1 (or better, drop the generic
compatible altogether).

Ok. Ideally I think we'd even look up the specific SoC names from the
SVC rather than the compatible string. I just didn't have a good list
for those to put in the driver.

The list is in arch/powerpc/include/asm/mpc85xx.h but I don't know why we
need to convert it to a string in the first place.

quoted

quoted

quoted

+	/*
+	 * syscon devices default to little-endian, but on powerpc we
have
+	 * existing device trees with big-endian maps and an absent
endianess
+	 * "big-property"
+	 */
+	if (!IS_ENABLED(CONFIG_POWERPC) &&
+	    !of_property_read_bool(dev->of_node, "big-endian"))
+		guts->little_endian = true;

This is not a syscon device (Yangbo's patch to add a guts node on
ls2080 is the only guts node that says "syscon", and that was a
leftover from earlier revisions and should probably be removed).
Even if it were, where is it documented that syscon defaults to
little-endian?

Documentation/devicetree/bindings/regmap/regmap.txt

We had a little screwup here, basically regmap (and by consequence,
syscon) always defaulted to little-endian way before that was
documented, so it's too late to change it,

What causes a device node to fall under the jurisdiction of regmap.txt?
 Again, these nodes do not claim "syscon" compatibility.

quoted

although I agree it would have made sense to document regmap to
default to big-endian on powerpc.

Please don't.  It's enough of a mess as is; no need to start throwing in
architecture ifdefs.

quoted

quoted

Documentation/devicetree/bindings/common-properties.txt says that
the individual binding specifies the default.  The default for this
node should be big-endian because that's what existed before there
was a need to describe the endianness.  And we need an update to the
guts binding to specify that.

Good point. This proably means that specifying both the "guts" and

"syscon"

quoted

compatible strings implies having to also specify the endianess
explicitly both ways, because otherwise we break one of the two

bindings.

Yes, but the node should only specify "guts".

quoted

quoted

quoted

+
+	guts->regs = devm_ioremap_resource(dev, 0);
+	if (!guts->regs) {
+		ret = -ENOMEM;
+		kfree(guts);
+		goto out;
+	}
+
+	fsl_guts_init(dev, guts);
+	ret = 0;
+out:
+	return ret;
+}
+
+static struct platform_driver fsl_soc_guts = {
+	.probe = fsl_guts_probe,
+	.driver.of_match_table = fsl_guts_of_match, };
+
+module_platform_driver(fsl_soc_guts);

Again, this means that the information is not available during early
boot, such as in the clock driver.  Thus we would not be able to
convert clk -qoriq's direct mfspr(SPRN_SVR) into an
soc_device_match() (or anything else that makes use of this file),
nor would we be able to move its access of the guts RCW registers
into this driver.

Correct. Do we have a reason to convert the mfspr() though? I don't
really see an improvement over the current state if we do that,

Then should we drop this patchset and put a similar PPC ifdef in
drivers/mmc/host/sdhci-of-esdhc.c?

There's also the RCW access.  You said in the patch 4/4 discussion that
you di dn't like any random driver ioremapping the registers...

quoted

and for new devices
that might need the erratum workaround, we could add a DT property
that would be preferred to both.

It's unlikely that we would know the erratum exists at the time the
device tree is created.  We also generally don't have separate device
trees for each revision of a chip (and if we did, we'd have users that
use the wrong one).

-Scott