diff mbox

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

Message ID 5339448.D8rpq4ofAC@wuerfel (mailing list archive)
State Rejected, archived
Delegated to: Stephen Boyd
Headers show

Commit Message

Arnd Bergmann May 30, 2016, 1:15 p.m. UTC
From: Yangbo Lu <yangbo.lu@nxp.com>

The global utilities block controls power management, I/O device
enabling, power-onreset(POR) configuration monitoring, alternate
function selection for multiplexed signals,and clock control.

This patch adds GUTS driver to manage and access global utilities
block.

[arnd turned this into a platform_driver registering a soc_device
 rather than providing an ad-hoc interface for soc-id]

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Crystal Wood June 2, 2016, 1:47 a.m. UTC | #1
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?

> +	bool little_endian;
> +	struct soc_device_attribute soc;
> +};
> +
> +static u32 fsl_guts_get_svr(struct guts *guts)
> +{
> +	if (guts->little_endian)
> +		return ioread32(guts->regs + GUTS_SVR);
> +	else
> +		return ioread32be(guts->regs + GUTS_SVR);
> +}
> +
> +static u32 fsl_guts_get_pvr(struct guts *guts)
> +{
> +	if (guts->little_endian)
> +		return ioread32(guts->regs + GUTS_PVR);
> +	else
> +		return ioread32be(guts->regs + GUTS_PVR);
> +}

You've removed the fallback to mfspr() on PPC, which would be helpful in some
virtualized environments where we don't have the guts node (but do have other
directly assigned devices).  Of course, this is a consequence of the
conversion into a platform device.

> +
> +/*
> + * 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).

> +	/*
> +	 * 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/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.

> +
> +	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.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 2, 2016, 8:43 a.m. UTC | #2
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.

> > +	bool little_endian;
> > +	struct soc_device_attribute soc;
> > +};
> > +
> > +static u32 fsl_guts_get_svr(struct guts *guts)
> > +{
> > +	if (guts->little_endian)
> > +		return ioread32(guts->regs + GUTS_SVR);
> > +	else
> > +		return ioread32be(guts->regs + GUTS_SVR);
> > +}
> > +
> > +static u32 fsl_guts_get_pvr(struct guts *guts)
> > +{
> > +	if (guts->little_endian)
> > +		return ioread32(guts->regs + GUTS_PVR);
> > +	else
> > +		return ioread32be(guts->regs + GUTS_PVR);
> > +}
> 
> You've removed the fallback to mfspr() on PPC, which would be helpful in some
> virtualized environments where we don't have the guts node (but do have other
> directly assigned devices).  Of course, this is a consequence of the
> conversion into a platform device.

Right, it just didn't make any sense after the conversion. I couldn't
figure out what the intention was for the fallback. Virtualized environments
are an interesting case, but I'd also argue that a virtualized system is
not the original SoC anyway, so reporting the physical SoC as the
main system in the guest is a bit strange and we probably want to
come up with a different solution there.

In soc_device, we probably want to just report the type of hypervisor
in this case, which is what most users would expect there. For the
specific case of the mmc driver (are there any real use cases where
you'd pass on the mmc controller to a guest, as opposed to passing either
an emulated block device or the mmc device on an emulated mmc host?),
that means we have to come up with a different solution, but then
we'd be free to work around this by modifying the DT node of the mmc
device.

> > +/*
> > + * 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.

> > +	/*
> > +	 * 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, although I agree it would have made sense to document
regmap to default to big-endian on powerpc.

> 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.

> > +
> > +	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, and for new devices
that might need the erratum workaround, we could add a DT property that would
be preferred to both.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Crystal Wood June 11, 2016, 1:50 a.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu June 23, 2016, 2:46 a.m. UTC | #4
SGkgQXJuZCwNCg0KQ291bGQgeW91IGNvbW1lbnQgb24gdGhlc2U/DQpUaGFua3MuDQoNCg0KQmVz
dCByZWdhcmRzLA0KWWFuZ2JvIEx1DQoNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0K
PiBGcm9tOiBTY290dCBXb29kIFttYWlsdG86b3NzQGJ1c2Vycm9yLm5ldF0NCj4gU2VudDogU2F0
dXJkYXksIEp1bmUgMTEsIDIwMTYgOTo1MSBBTQ0KPiBUbzogQXJuZCBCZXJnbWFubjsgbGludXhw
cGMtZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gQ2M6IE1hcmsgUnV0bGFuZDsgVWxmIEhhbnNzb247
IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LQ0KPiBpMmNAdmdlci5rZXJuZWwu
b3JnOyBsaW51eC1jbGtAdmdlci5rZXJuZWwub3JnOyBRaWFuZyBaaGFvOyBSdXNzZWxsIEtpbmc7
DQo+IEJodXBlc2ggU2hhcm1hOyBKb2VyZyBSb2VkZWw7IENsYXVkaXUgTWFub2lsOyBkZXZpY2V0
cmVlQHZnZXIua2VybmVsLm9yZzsNCj4gS3VtYXIgR2FsYTsgUm9iIEhlcnJpbmc7IFNhbnRvc2gg
U2hpbGlta2FyOyBsaW51eC1hcm0tDQo+IGtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBuZXRk
ZXZAdmdlci5rZXJuZWwub3JnOyBsaW51eC0NCj4gbW1jQHZnZXIua2VybmVsLm9yZzsgWGlhb2Jv
IFhpZTsgWWFuZy1MZW8gTGk7IGlvbW11QGxpc3RzLmxpbnV4LQ0KPiBmb3VuZGF0aW9uLm9yZzsg
WWFuZ2JvIEx1DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMi80XSBzb2M6IGZzbDogYWRkIEdVVFMg
ZHJpdmVyIGZvciBRb3JJUSBwbGF0Zm9ybXMNCj4gDQo+IE9uIFRodSwgMjAxNi0wNi0wMiBhdCAx
MDo0MyArMDIwMCwgQXJuZCBCZXJnbWFubiB3cm90ZToNCj4gPiBPbiBXZWRuZXNkYXksIEp1bmUg
MSwgMjAxNiA4OjQ3OjIyIFBNIENFU1QgU2NvdHQgV29vZCB3cm90ZToNCj4gPiA+IE9uIE1vbiwg
MjAxNi0wNS0zMCBhdCAxNToxNSArMDIwMCwgQXJuZCBCZXJnbWFubiB3cm90ZToNCj4gPiA+ID4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvc29jL2ZzbC9ndXRzLmMgYi9kcml2ZXJzL3NvYy9mc2wvZ3V0
cy5jIG5ldw0KPiA+ID4gPiBmaWxlIG1vZGUgMTAwNjQ0IGluZGV4IDAwMDAwMDAwMDAwMC4uMmYz
MDY5OGY1YmNmDQo+ID4gPiA+IC0tLSAvZGV2L251bGwNCj4gPiA+ID4gKysrIGIvZHJpdmVycy9z
b2MvZnNsL2d1dHMuYw0KPiA+ID4gPiBAQCAtMCwwICsxLDEzMCBAQA0KPiA+ID4gPiArLyoNCj4g
PiA+ID4gKyAqIEZyZWVzY2FsZSBRb3JJUSBQbGF0Zm9ybXMgR1VUUyBEcml2ZXINCj4gPiA+ID4g
KyAqDQo+ID4gPiA+ICsgKiBDb3B5cmlnaHQgKEMpIDIwMTYgRnJlZXNjYWxlIFNlbWljb25kdWN0
b3IsIEluYy4NCj4gPiA+ID4gKyAqDQo+ID4gPiA+ICsgKiBUaGlzIHByb2dyYW0gaXMgZnJlZSBz
b2Z0d2FyZTsgeW91IGNhbiByZWRpc3RyaWJ1dGUgaXQgYW5kL29yDQo+ID4gPiA+ICttb2RpZnkN
Cj4gPiA+ID4gKyAqIGl0IHVuZGVyIHRoZSB0ZXJtcyBvZiB0aGUgR05VIEdlbmVyYWwgUHVibGlj
IExpY2Vuc2UgYXMNCj4gPiA+ID4gK3B1Ymxpc2hlZCBieQ0KPiA+ID4gPiArICogdGhlIEZyZWUg
U29mdHdhcmUgRm91bmRhdGlvbjsgZWl0aGVyIHZlcnNpb24gMiBvZiB0aGUgTGljZW5zZSwNCj4g
PiA+ID4gK29yDQo+ID4gPiA+ICsgKiAoYXQgeW91ciBvcHRpb24pIGFueSBsYXRlciB2ZXJzaW9u
Lg0KPiA+ID4gPiArICovDQo+ID4gPiA+ICsNCj4gPiA+ID4gKyNpbmNsdWRlIDxsaW51eC9pby5o
Pg0KPiA+ID4gPiArI2luY2x1ZGUgPGxpbnV4L3BsYXRmb3JtX2RldmljZS5oPiAjaW5jbHVkZSA8
bGludXgvbW9kdWxlLmg+DQo+ID4gPiA+ICsjaW5jbHVkZSA8bGludXgvc2xhYi5oPiAjaW5jbHVk
ZSA8bGludXgvb2ZfYWRkcmVzcy5oPiAjaW5jbHVkZQ0KPiA+ID4gPiArPGxpbnV4L29mX3BsYXRm
b3JtLmg+ICNpbmNsdWRlIDxsaW51eC9zeXNfc29jLmg+DQo+ID4gPiA+ICsNCj4gPiA+ID4gKyNk
ZWZpbmUgR1VUU19QVlIJMHgwYTANCj4gPiA+ID4gKyNkZWZpbmUgR1VUU19TVlIJMHgwYTQNCj4g
PiA+ID4gKw0KPiA+ID4gPiArc3RydWN0IGd1dHMgew0KPiA+ID4gPiArCXZvaWQgX19pb21lbSAq
cmVnczsNCj4gPiA+DQo+ID4gPiBXZSBhbHJlYWR5IGhhdmUgYSBzdHJ1Y3QgdG8gZGVmaW5lIGd1
dHMuICBXaHkgYXJlIHlvdSBub3QgdXNpbmcgaXQ/DQo+ID4gPiBXaHkgZG8geW91IGNvbnNpZGVy
IHVzaW5nIGl0IHRvIGJlICJhYnVzZSI/ICBXaGF0IGlmIHdlIHdhbnQgdG8gbW92ZQ0KPiA+ID4g
bW9yZSBndXRzIGZ1bmN0aW9uYWxpdHkgaW50byB0aGlzIGRyaXZlcj8NCj4gPg0KPiA+IFRoaXMg
c3RydWN0dXJlIHdhcyBpbiB0aGUgb3JpZ2luYWwgcGF0Y2gsIEkgbGVmdCBpdCBpbiB0aGVyZSwg
b25seQ0KPiA+IHJlbW92ZWQgdGhlIGluY2x1c2lvbiBvZiB0aGUgcG93ZXJwYyBoZWFkZXIgZmls
ZSwgd2hpY2ggc2VlbWVkIHRvIGJlDQo+ID4gbWlzcGxhY2VkLg0KPiANCj4gSSdtIG5vdCByZWZl
cmluZyAic3RydWN0IGd1dHMiLiAgSSdtIHJlZmVycmluZyB0byBjaGFuZ2luZyAic3RydWN0DQo+
IGNjc3JfZ3V0cyBfX2lvbWVtICpyZWdzIiBpbnRvICJ2b2lkIF9faW9tZW0gKnJlZ3MiLg0KPiAN
Cj4gQW5kIGl0J3Mgbm90IGEgcG93ZXJwYyBoZWFkZXIgZmlsZS4NCj4gDQo+ID4gPiA+ICsvKg0K
PiA+ID4gPiArICogVGFibGUgZm9yIG1hdGNoaW5nIGNvbXBhdGlibGUgc3RyaW5ncywgZm9yIGRl
dmljZSB0cmVlDQo+ID4gPiA+ICsgKiBndXRzIG5vZGUsIGZvciBGcmVlc2NhbGUgUW9ySVEgU09D
cy4NCj4gPiA+ID4gKyAqLw0KPiA+ID4gPiArc3RhdGljIGNvbnN0IHN0cnVjdCBvZl9kZXZpY2Vf
aWQgZnNsX2d1dHNfb2ZfbWF0Y2hbXSA9IHsNCj4gPiA+ID4gKwkvKiBGb3IgVDQgJiBCNCBTZXJp
ZXMgU09DcyAqLw0KPiA+ID4gPiArCXsgLmNvbXBhdGlibGUgPSAiZnNsLHFvcmlxLWRldmljZS1j
b25maWctMS4wIiwgLmRhdGEgPSAiVDQvQjQNCj4gPiA+ID4gc2VyaWVzIiB9LA0KPiA+ID4gW3Nu
aXBdDQo+ID4gPiA+ICsJeyAuY29tcGF0aWJsZSA9ICJmc2wscW9yaXEtZGV2aWNlLWNvbmZpZy0y
LjAiLCAuZGF0YSA9ICJQDQo+ID4gPiA+IHNlcmllcyINCj4gPiA+DQo+ID4gPiBBcyBub3RlZCBp
biBteSBjb21tZW50IG9uIHBhdGNoIDMvNCwgdGhlc2UgZGVzY3JpcHRpb25zIGFyZSByZXZlcnNl
ZC4NCj4gPiA+DQo+ID4gPiBUaGV5J3JlIGFsc28gaW5jb21wbGV0ZS4gIHQyMDgwIGhhcyBkZXZp
Y2UgY29uZmlnIDIuMC4gIHQxMDQwIGlzDQo+ID4gPiBkZXNjcmliZWQgYXMNCj4gPiA+IDIuMCB0
aG91Z2ggaXQgc2hvdWxkIHByb2JhYmx5IGJlIDIuMSAob3IgYmV0dGVyLCBkcm9wIHRoZSBnZW5l
cmljDQo+ID4gPiBjb21wYXRpYmxlIGFsdG9nZXRoZXIpLg0KPiA+DQo+ID4gT2suIElkZWFsbHkg
SSB0aGluayB3ZSdkIGV2ZW4gbG9vayB1cCB0aGUgc3BlY2lmaWMgU29DIG5hbWVzIGZyb20gdGhl
DQo+ID4gU1ZDIHJhdGhlciB0aGFuIHRoZSBjb21wYXRpYmxlIHN0cmluZy4gSSBqdXN0IGRpZG4n
dCBoYXZlIGEgZ29vZCBsaXN0DQo+ID4gZm9yIHRob3NlIHRvIHB1dCBpbiB0aGUgZHJpdmVyLg0K
PiANCj4gVGhlIGxpc3QgaXMgaW4gYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL21wYzg1eHguaCBi
dXQgSSBkb24ndCBrbm93IHdoeSB3ZQ0KPiBuZWVkIHRvIGNvbnZlcnQgaXQgdG8gYSBzdHJpbmcg
aW4gdGhlIGZpcnN0IHBsYWNlLg0KPiANCj4gPg0KPiA+ID4gPiArCS8qDQo+ID4gPiA+ICsJICog
c3lzY29uIGRldmljZXMgZGVmYXVsdCB0byBsaXR0bGUtZW5kaWFuLCBidXQgb24gcG93ZXJwYyB3
ZQ0KPiA+ID4gPiBoYXZlDQo+ID4gPiA+ICsJICogZXhpc3RpbmcgZGV2aWNlIHRyZWVzIHdpdGgg
YmlnLWVuZGlhbiBtYXBzIGFuZCBhbiBhYnNlbnQNCj4gPiA+ID4gZW5kaWFuZXNzDQo+ID4gPiA+
ICsJICogImJpZy1wcm9wZXJ0eSINCj4gPiA+ID4gKwkgKi8NCj4gPiA+ID4gKwlpZiAoIUlTX0VO
QUJMRUQoQ09ORklHX1BPV0VSUEMpICYmDQo+ID4gPiA+ICsJICAgICFvZl9wcm9wZXJ0eV9yZWFk
X2Jvb2woZGV2LT5vZl9ub2RlLCAiYmlnLWVuZGlhbiIpKQ0KPiA+ID4gPiArCQlndXRzLT5saXR0
bGVfZW5kaWFuID0gdHJ1ZTsNCj4gPiA+DQo+ID4gPiBUaGlzIGlzIG5vdCBhIHN5c2NvbiBkZXZp
Y2UgKFlhbmdibydzIHBhdGNoIHRvIGFkZCBhIGd1dHMgbm9kZSBvbg0KPiA+ID4gbHMyMDgwIGlz
IHRoZSBvbmx5IGd1dHMgbm9kZSB0aGF0IHNheXMgInN5c2NvbiIsIGFuZCB0aGF0IHdhcyBhDQo+
ID4gPiBsZWZ0b3ZlciBmcm9tIGVhcmxpZXIgcmV2aXNpb25zIGFuZCBzaG91bGQgcHJvYmFibHkg
YmUgcmVtb3ZlZCkuDQo+ID4gPiBFdmVuIGlmIGl0IHdlcmUsIHdoZXJlIGlzIGl0IGRvY3VtZW50
ZWQgdGhhdCBzeXNjb24gZGVmYXVsdHMgdG8NCj4gPiA+IGxpdHRsZS1lbmRpYW4/DQo+ID4NCj4g
PiBEb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvcmVnbWFwL3JlZ21hcC50eHQNCj4g
Pg0KPiA+IFdlIGhhZCBhIGxpdHRsZSBzY3Jld3VwIGhlcmUsIGJhc2ljYWxseSByZWdtYXAgKGFu
ZCBieSBjb25zZXF1ZW5jZSwNCj4gPiBzeXNjb24pIGFsd2F5cyBkZWZhdWx0ZWQgdG8gbGl0dGxl
LWVuZGlhbiB3YXkgYmVmb3JlIHRoYXQgd2FzDQo+ID4gZG9jdW1lbnRlZCwgc28gaXQncyB0b28g
bGF0ZSB0byBjaGFuZ2UgaXQsDQo+IA0KPiBXaGF0IGNhdXNlcyBhIGRldmljZSBub2RlIHRvIGZh
bGwgdW5kZXIgdGhlIGp1cmlzZGljdGlvbiBvZiByZWdtYXAudHh0Pw0KPiAgQWdhaW4sIHRoZXNl
IG5vZGVzIGRvIG5vdCBjbGFpbSAic3lzY29uIiBjb21wYXRpYmlsaXR5Lg0KPiANCj4gPiBhbHRo
b3VnaCBJIGFncmVlIGl0IHdvdWxkIGhhdmUgbWFkZSBzZW5zZSB0byBkb2N1bWVudCByZWdtYXAg
dG8NCj4gPiBkZWZhdWx0IHRvIGJpZy1lbmRpYW4gb24gcG93ZXJwYy4NCj4gDQo+IFBsZWFzZSBk
b24ndC4gIEl0J3MgZW5vdWdoIG9mIGEgbWVzcyBhcyBpczsgbm8gbmVlZCB0byBzdGFydCB0aHJv
d2luZyBpbg0KPiBhcmNoaXRlY3R1cmUgaWZkZWZzLg0KPiANCj4gPiA+IERvY3VtZW50YXRpb24v
ZGV2aWNldHJlZS9iaW5kaW5ncy9jb21tb24tcHJvcGVydGllcy50eHQgc2F5cyB0aGF0DQo+ID4g
PiB0aGUgaW5kaXZpZHVhbCBiaW5kaW5nIHNwZWNpZmllcyB0aGUgZGVmYXVsdC4gIFRoZSBkZWZh
dWx0IGZvciB0aGlzDQo+ID4gPiBub2RlIHNob3VsZCBiZSBiaWctZW5kaWFuIGJlY2F1c2UgdGhh
dCdzIHdoYXQgZXhpc3RlZCBiZWZvcmUgdGhlcmUNCj4gPiA+IHdhcyBhIG5lZWQgdG8gZGVzY3Jp
YmUgdGhlIGVuZGlhbm5lc3MuICBBbmQgd2UgbmVlZCBhbiB1cGRhdGUgdG8gdGhlDQo+ID4gPiBn
dXRzIGJpbmRpbmcgdG8gc3BlY2lmeSB0aGF0Lg0KPiA+DQo+ID4gR29vZCBwb2ludC4gVGhpcyBw
cm9hYmx5IG1lYW5zIHRoYXQgc3BlY2lmeWluZyBib3RoIHRoZSAiZ3V0cyIgYW5kDQo+ICJzeXNj
b24iDQo+ID4gY29tcGF0aWJsZSBzdHJpbmdzIGltcGxpZXMgaGF2aW5nIHRvIGFsc28gc3BlY2lm
eSB0aGUgZW5kaWFuZXNzDQo+ID4gZXhwbGljaXRseSBib3RoIHdheXMsIGJlY2F1c2Ugb3RoZXJ3
aXNlIHdlIGJyZWFrIG9uZSBvZiB0aGUgdHdvDQo+IGJpbmRpbmdzLg0KPiANCj4gWWVzLCBidXQg
dGhlIG5vZGUgc2hvdWxkIG9ubHkgc3BlY2lmeSAiZ3V0cyIuDQo+IA0KPiA+DQo+ID4gPiA+ICsN
Cj4gPiA+ID4gKwlndXRzLT5yZWdzID0gZGV2bV9pb3JlbWFwX3Jlc291cmNlKGRldiwgMCk7DQo+
ID4gPiA+ICsJaWYgKCFndXRzLT5yZWdzKSB7DQo+ID4gPiA+ICsJCXJldCA9IC1FTk9NRU07DQo+
ID4gPiA+ICsJCWtmcmVlKGd1dHMpOw0KPiA+ID4gPiArCQlnb3RvIG91dDsNCj4gPiA+ID4gKwl9
DQo+ID4gPiA+ICsNCj4gPiA+ID4gKwlmc2xfZ3V0c19pbml0KGRldiwgZ3V0cyk7DQo+ID4gPiA+
ICsJcmV0ID0gMDsNCj4gPiA+ID4gK291dDoNCj4gPiA+ID4gKwlyZXR1cm4gcmV0Ow0KPiA+ID4g
PiArfQ0KPiA+ID4gPiArDQo+ID4gPiA+ICtzdGF0aWMgc3RydWN0IHBsYXRmb3JtX2RyaXZlciBm
c2xfc29jX2d1dHMgPSB7DQo+ID4gPiA+ICsJLnByb2JlID0gZnNsX2d1dHNfcHJvYmUsDQo+ID4g
PiA+ICsJLmRyaXZlci5vZl9tYXRjaF90YWJsZSA9IGZzbF9ndXRzX29mX21hdGNoLCB9Ow0KPiA+
ID4gPiArDQo+ID4gPiA+ICttb2R1bGVfcGxhdGZvcm1fZHJpdmVyKGZzbF9zb2NfZ3V0cyk7DQo+
ID4gPg0KPiA+ID4gQWdhaW4sIHRoaXMgbWVhbnMgdGhhdCB0aGUgaW5mb3JtYXRpb24gaXMgbm90
IGF2YWlsYWJsZSBkdXJpbmcgZWFybHkNCj4gPiA+IGJvb3QsIHN1Y2ggYXMgaW4gdGhlIGNsb2Nr
IGRyaXZlci4gIFRodXMgd2Ugd291bGQgbm90IGJlIGFibGUgdG8NCj4gPiA+IGNvbnZlcnQgY2xr
IC1xb3JpcSdzIGRpcmVjdCBtZnNwcihTUFJOX1NWUikgaW50byBhbg0KPiA+ID4gc29jX2Rldmlj
ZV9tYXRjaCgpIChvciBhbnl0aGluZyBlbHNlIHRoYXQgbWFrZXMgdXNlIG9mIHRoaXMgZmlsZSks
DQo+ID4gPiBub3Igd291bGQgd2UgYmUgYWJsZSB0byBtb3ZlIGl0cyBhY2Nlc3Mgb2YgdGhlIGd1
dHMgUkNXIHJlZ2lzdGVycw0KPiA+ID4gaW50byB0aGlzIGRyaXZlci4NCj4gPg0KPiA+IENvcnJl
Y3QuIERvIHdlIGhhdmUgYSByZWFzb24gdG8gY29udmVydCB0aGUgbWZzcHIoKSB0aG91Z2g/IEkg
ZG9uJ3QNCj4gPiByZWFsbHkgc2VlIGFuIGltcHJvdmVtZW50IG92ZXIgdGhlIGN1cnJlbnQgc3Rh
dGUgaWYgd2UgZG8gdGhhdCwNCj4gDQo+IFRoZW4gc2hvdWxkIHdlIGRyb3AgdGhpcyBwYXRjaHNl
dCBhbmQgcHV0IGEgc2ltaWxhciBQUEMgaWZkZWYgaW4NCj4gZHJpdmVycy9tbWMvaG9zdC9zZGhj
aS1vZi1lc2RoYy5jPw0KPiANCj4gVGhlcmUncyBhbHNvIHRoZSBSQ1cgYWNjZXNzLiAgWW91IHNh
aWQgaW4gdGhlIHBhdGNoIDQvNCBkaXNjdXNzaW9uIHRoYXQNCj4geW91IGRpIGRuJ3QgbGlrZSBh
bnkgcmFuZG9tIGRyaXZlciBpb3JlbWFwcGluZyB0aGUgcmVnaXN0ZXJzLi4uDQo+IA0KPiA+IGFu
ZCBmb3IgbmV3IGRldmljZXMNCj4gPiB0aGF0IG1pZ2h0IG5lZWQgdGhlIGVycmF0dW0gd29ya2Fy
b3VuZCwgd2UgY291bGQgYWRkIGEgRFQgcHJvcGVydHkNCj4gPiB0aGF0IHdvdWxkIGJlIHByZWZl
cnJlZCB0byBib3RoLg0KPiANCj4gSXQncyB1bmxpa2VseSB0aGF0IHdlIHdvdWxkIGtub3cgdGhl
IGVycmF0dW0gZXhpc3RzIGF0IHRoZSB0aW1lIHRoZQ0KPiBkZXZpY2UgdHJlZSBpcyBjcmVhdGVk
LiAgV2UgYWxzbyBnZW5lcmFsbHkgZG9uJ3QgaGF2ZSBzZXBhcmF0ZSBkZXZpY2UNCj4gdHJlZXMg
Zm9yIGVhY2ggcmV2aXNpb24gb2YgYSBjaGlwIChhbmQgaWYgd2UgZGlkLCB3ZSdkIGhhdmUgdXNl
cnMgdGhhdA0KPiB1c2UgdGhlIHdyb25nIG9uZSkuDQo+IA0KPiAtU2NvdHQNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu July 7, 2016, 2:35 a.m. UTC | #5
Hi Arnd,

Could you reply when you see the email?
If your method doesn’t resolve the problem, we still want to use our old patchset.

This guts driver had been discussed about one year and blocked many workaround upstream.
So please help to review and comment soon.

Thanks a lot.


Best regards,
Yangbo Lu

> -----Original Message-----

> From: Yangbo Lu

> Sent: Thursday, June 23, 2016 10:46 AM

> To: 'Scott Wood'; Arnd Bergmann; linuxppc-dev@lists.ozlabs.org

> Cc: Mark Rutland; Ulf Hansson; linux-kernel@vger.kernel.org; linux-

> i2c@vger.kernel.org; linux-clk@vger.kernel.org; Qiang Zhao; Russell King;

> Bhupesh Sharma; Joerg Roedel; Claudiu Manoil; devicetree@vger.kernel.org;

> Kumar Gala; Rob Herring; Santosh Shilimkar; linux-arm-

> kernel@lists.infradead.org; netdev@vger.kernel.org; linux-

> mmc@vger.kernel.org; Xiaobo Xie; Yang-Leo Li; iommu@lists.linux-

> foundation.org

> Subject: RE: [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@buserror.net]

> > Sent: Saturday, June 11, 2016 9:51 AM

> > To: Arnd Bergmann; linuxppc-dev@lists.ozlabs.org

> > Cc: Mark Rutland; Ulf Hansson; linux-kernel@vger.kernel.org; linux-

> > i2c@vger.kernel.org; linux-clk@vger.kernel.org; Qiang Zhao; Russell

> > King; Bhupesh Sharma; Joerg Roedel; Claudiu Manoil;

> > devicetree@vger.kernel.org; Kumar Gala; Rob Herring; Santosh

> > Shilimkar; linux-arm- kernel@lists.infradead.org;

> > netdev@vger.kernel.org; linux- mmc@vger.kernel.org; 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
Arnd Bergmann July 7, 2016, 8:30 a.m. UTC | #6
On Thursday, July 7, 2016 2:35:33 AM CEST Yangbo Lu wrote:
> Hi Arnd,
> 
> Could you reply when you see the email?
> If your method doesn’t resolve the problem, we still want to use our old patchset.
> 
> This guts driver had been discussed about one year and blocked many workaround upstream.
> So please help to review and comment soon.
> 

I don't really see how more discussion is going to help us here. I think
I've made it pretty clear that I don't want to see another platform
specific way to read an SoC revision and I've even sent a proof-of-concept
patch to show how the interface can work, now it's up to you to fit the
guts hardware into that and send a new patch series.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Crystal Wood July 7, 2016, 8:42 p.m. UTC | #7
On Thu, 2016-07-07 at 10:30 +0200, Arnd Bergmann wrote:
> On Thursday, July 7, 2016 2:35:33 AM CEST Yangbo Lu wrote:
> > 
> > Hi Arnd,
> > 
> > Could you reply when you see the email?
> > If your method doesn’t resolve the problem, we still want to use our old
> > patchset.
> > 
> > This guts driver had been discussed about one year and blocked many
> > workaround upstream.
> > So please help to review and comment soon.
> > 
> I don't really see how more discussion is going to help us here. I think
> I've made it pretty clear that I don't want to see another platform
> specific way to read an SoC revision and I've even sent a proof-of-concept
> patch to show how the interface can work, now it's up to you to fit the
> guts hardware into that and send a new patch series.

In which relevant maintainership capacity are you NACKing it?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu July 8, 2016, 3:05 a.m. UTC | #8
Hi Arnd,


> -----Original Message-----

> From: Arnd Bergmann [mailto:arnd@arndb.de]

> Sent: Thursday, July 07, 2016 4:30 PM

> To: Yangbo Lu

> Cc: Scott Wood; linuxppc-dev@lists.ozlabs.org; Mark Rutland; Ulf Hansson;

> linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-

> clk@vger.kernel.org; Qiang Zhao; Russell King; Bhupesh Sharma; Joerg

> Roedel; Claudiu Manoil; devicetree@vger.kernel.org; Kumar Gala; Rob

> Herring; Santosh Shilimkar; linux-arm-kernel@lists.infradead.org;

> netdev@vger.kernel.org; linux-mmc@vger.kernel.org; Xiaobo Xie; Yang-Leo

> Li; iommu@lists.linux-foundation.org

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

> 

> On Thursday, July 7, 2016 2:35:33 AM CEST Yangbo Lu wrote:

> > Hi Arnd,

> >

> > Could you reply when you see the email?

> > If your method doesn’t resolve the problem, we still want to use our

> old patchset.

> >

> > This guts driver had been discussed about one year and blocked many

> workaround upstream.

> > So please help to review and comment soon.

> >

> 

> I don't really see how more discussion is going to help us here. I think

> I've made it pretty clear that I don't want to see another platform

> specific way to read an SoC revision and I've even sent a proof-of-

> concept patch to show how the interface can work, now it's up to you to

> fit the guts hardware into that and send a new patch series.

> 

> 	Arnd


I think your proof-of-concept patch is still in discussion. Some answers are needed from you to
address Scott's comments on your patchset. Have you reached an agreement?

Thanks.

- Yangbo Lu
diff mbox

Patch

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index cb58ef0d9b2c..7106463f118e 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -2,7 +2,7 @@  menu "SOC (System On Chip) specific Drivers"
 
 source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/brcmstb/Kconfig"
-source "drivers/soc/fsl/qe/Kconfig"
+source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/rockchip/Kconfig"
diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
new file mode 100644
index 000000000000..33d331cac8d6
--- /dev/null
+++ b/drivers/soc/fsl/Kconfig
@@ -0,0 +1,15 @@ 
+#
+# Freescale SOC drivers
+#
+
+source "drivers/soc/fsl/qe/Kconfig"
+
+config FSL_GUTS
+	bool "NXP Layerscale SoC identification"
+	depends on PPC_FSL || SOC_LS1021A || ARCH_LAYERSCAPE || COMPILE_TEST
+	default PPC_FSL || SOC_LS1021A || ARCH_LAYERSCAPE
+	select SOC_BUS
+	help
+	  This registers a SoC device for NXP (formerly Freescale)
+	  Layerscape devices, making information about the system
+	  available in /sys/devices/soc/
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
index 203307fd92c1..02afb7f980f6 100644
--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile
@@ -4,3 +4,4 @@ 
 
 obj-$(CONFIG_QUICC_ENGINE)		+= qe/
 obj-$(CONFIG_CPM)			+= qe/
+obj-$(CONFIG_FSL_GUTS)			+= guts.o
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;
+	bool little_endian;
+	struct soc_device_attribute soc;
+};
+
+static u32 fsl_guts_get_svr(struct guts *guts)
+{
+	if (guts->little_endian)
+		return ioread32(guts->regs + GUTS_SVR);
+	else
+		return ioread32be(guts->regs + GUTS_SVR);
+}
+
+static u32 fsl_guts_get_pvr(struct guts *guts)
+{
+	if (guts->little_endian)
+		return ioread32(guts->regs + GUTS_PVR);
+	else
+		return ioread32be(guts->regs + GUTS_PVR);
+}
+
+/*
+ * 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" },
+	/* For P Series SOCs */
+	{ .compatible = "fsl,p1010-guts", .data = "P1010/P1014" },
+	{ .compatible = "fsl,p1020-guts", .data = "P1020/P1011" },
+	{ .compatible = "fsl,p1021-guts", .data = "P1021/P1012" },
+	{ .compatible = "fsl,p1022-guts", .data = "P1022/P1013" },
+	{ .compatible = "fsl,p1023-guts", .data = "P1013/P1017" },
+	{ .compatible = "fsl,p2020-guts", .data = "P2010/P2020" },
+	{ .compatible = "fsl,qoriq-device-config-2.0", .data = "P series" },
+	/* For BSC Series SOCs */
+	{ .compatible = "fsl,bsc9131-guts", .data = "BSC9131 Qonverge" },
+	{ .compatible = "fsl,bsc9132-guts", .data = "BSC9132 Qonverge" },
+	/* For MPC85xx Series SOCs */
+	{ .compatible = "fsl,mpc8536-guts", .data = "PowerPC MPC8536" },
+	{ .compatible = "fsl,mpc8544-guts", .data = "PowerPC MPC8544" },
+	{ .compatible = "fsl,mpc8548-guts", .data = "PowerPC MPC8548" },
+	{ .compatible = "fsl,mpc8568-guts", .data = "PowerPC MPC8568" },
+	{ .compatible = "fsl,mpc8569-guts", .data = "PowerPC MPC8569" },
+	{ .compatible = "fsl,mpc8572-guts", .data = "PowerPC MPC8572" },
+	/* For Layerscape Series SOCs */
+	{ .compatible = "fsl,ls1021a-dcfg", .data = "Layerscape LS1021A" },
+	{ .compatible = "fsl,ls1043a-dcfg", .data = "Layerscape LS1043A" },
+	{ .compatible = "fsl,ls2080a-dcfg", .data = "Layerscape LS2080A" },
+	{}
+};
+
+static void fsl_guts_init(struct device *dev, struct guts *guts)
+{
+	const struct of_device_id *id;
+	u32 svr = fsl_guts_get_svr(guts);
+
+	guts->soc.family = "NXP QorIQ";
+	id = of_match_node(fsl_guts_of_match, dev->of_node);
+	guts->soc.soc_id = devm_kasprintf(dev, "%s (ver 0x%06x)" id->data,
+					  svr >> 8;
+	guts->soc.revision = devm_kasprintf(dev, GFP_KERNEL, "0x%02x",
+					    svr & 0xff);
+}
+
+static int fsl_guts_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct guts *guts;
+	int ret;
+
+	guts = devm_kzalloc(dev, sizeof(*guts), GFP_KERNEL);
+	if (!guts) {
+		ret = -ENOMEM;
+		goto out;
+
+	}
+
+	/*
+	 * 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;
+
+	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);