Hi Arnd, Could you comment on these? Thanks. Best regards, Yangbo Lu > -----Original Message----- > From: Scott Wood [mailto:oss@xxxxxxxxxxxx] > Sent: Saturday, June 11, 2016 9:51 AM > To: Arnd Bergmann; linuxppc-dev@xxxxxxxxxxxxxxxx > Cc: Mark Rutland; Ulf Hansson; linux-kernel@xxxxxxxxxxxxxxx; linux- > i2c@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; Qiang Zhao; Russell King; > Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree@xxxxxxxxxxxxxxx; > Kumar Gala; Rob Herring; Santosh Shilimkar; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux- > mmc@xxxxxxxxxxxxxxx; Xiaobo Xie; Yang-Leo Li; iommu@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: > > On Wednesday, June 1, 2016 8:47:22 PM CEST Scott Wood wrote: > > > On Mon, 2016-05-30 at 15:15 +0200, Arnd Bergmann wrote: > > > > 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. > > > > > +/* > > > > + * 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] > > > > + { .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. > > > > > > > + /* > > > > + * 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. > > > 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. > > > > 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" > > 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". > > > > > > > + > > > > + 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... > > > 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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
