* [v10, 7/7] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
2016-05-26 7:44 ` Ulf Hansson
@ 2016-05-30 13:13 ` Arnd Bergmann
2016-05-30 13:14 ` [PATCH 1/4] base: soc: introduce soc_device_match() interface Arnd Bergmann
` (3 subsequent siblings)
4 siblings, 0 replies; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 13:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday, May 26, 2016 9:44:10 AM CEST Ulf Hansson wrote:
> On 26 May 2016 at 06:05, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > Hi Uffe,
> >
> > Could we merge this patchset? ...
> > It has been a long time to wait for Arnd's response...
> >
> > Thanks a lot.
> >
> >
>
> As we are still in the merge window I won't queue anything but fixes.
> Let's give Arnd another week or so to respond.
I've got a patch series now that implements a method for matching
the soc ID, see the following emails.
Arnd
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 1/4] base: soc: introduce soc_device_match() interface
2016-05-26 7:44 ` Ulf Hansson
2016-05-30 13:13 ` Arnd Bergmann
@ 2016-05-30 13:14 ` Arnd Bergmann
2016-05-30 14:57 ` Arnd Bergmann
2016-05-30 13:15 ` [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Arnd Bergmann
` (2 subsequent siblings)
4 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 13:14 UTC (permalink / raw)
To: linux-arm-kernel
We keep running into cases where device drivers want to know the exact
version of the SoC a they are currently running on. In the past, this
has usually been done through a vendor specific API that can be called
by a driver, or by directly accessing some kind of version register
that is not part of the device itself but that belongs to a global
register area of the chip.
Common reasons for doing this include:
- A machine is not using devicetree or similar for passing data
about on-chip devices, but just announces their presence using
boot-time platform devices, and the machine code itself does
not care about the revision.
- There is existing firmware or boot loaders with existing DT
binaries with generic compatible strings that do not identify
the particular revision of each device, but the driver knows
which SoC revisions include which part
- A prerelease version of a chip has some quirks and we are
using the same version of the bootloader and the DT blob
on both the prerelease and the final version. An update of
the DT binding seems inappropriate because that would involve
maintaining multiple copies of the dts and/or bootloader.
This introduces the soc_device_match() interface that is meant
to work like of_match_node() but instead of identifying the
version of a device, it identifies the SoC itself using a
vendor-agnostic interface.
Unlike soc_device_match(), we do not do an exact string compare
but instead use glob_match() to allow wildcards in strings.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 246acdafedb6..fc7613cc7fd5 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
config SOC_BUS
bool
+ select GLOB
source "drivers/base/regmap/Kconfig"
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 75b98aad6faf..e9623c6674a5 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -14,6 +14,7 @@
#include <linux/spinlock.h>
#include <linux/sys_soc.h>
#include <linux/err.h>
+#include <linux/glob.h>
static DEFINE_IDA(soc_ida);
@@ -168,3 +169,60 @@ static void __exit soc_bus_unregister(void)
bus_unregister(&soc_bus_type);
}
module_exit(soc_bus_unregister);
+
+static int soc_device_match_one(struct device *dev, void *arg)
+{
+ struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+ struct soc_device_attribute *match = arg;
+
+ if (match->machine && !glob_match(match->machine, soc_dev->attr->machine))
+ return 0;
+
+ if (match->family && !glob_match(match->family, soc_dev->attr->family))
+ return 0;
+
+ if (match->revision && !glob_match(match->revision, soc_dev->attr->revision))
+ return 0;
+
+ if (match->soc_id && !glob_match(match->soc_id, soc_dev->attr->revision))
+ return 0;
+
+ return 1;
+}
+
+/*
+ * soc_device_match - identify the SoC in the machine
+ * @matches: zero-terminated array of possible matches
+ *
+ * returns the first matching entry of the argument array, or NULL
+ * if none of them match.
+ *
+ * This function is meant as a helper in place of of_match_node()
+ * in cases where either no device tree is available or the information
+ * in a device node is insufficient to identify a particular variant
+ * by its compatible strings or other properties. For new devices,
+ * the DT binding should always provide unique compatible strings
+ * that allow the use of of_match_node() instead.
+ *
+ * The calling function can use the .data entry of the
+ * soc_device_attribute to pass a structure or function pointer for
+ * each entry.
+ */
+struct soc_device_attribute *soc_device_match(struct soc_device_attribute *matches)
+{
+ struct device *dev;
+ int ret;
+
+ for (ret = 0; ret == 0; matches++) {
+ if (matches->machine || matches->family ||
+ matches->revision || matches->soc_id)
+ return NULL;
+
+ dev = NULL;
+ ret = bus_for_each_dev(&soc_bus_type, dev, matches,
+ soc_device_match_one);
+ }
+
+ return matches;
+}
+EXPORT_SYMBOL_GPL(soc_device_match);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index 2739ccb69571..02c48c76052b 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -13,6 +13,8 @@ struct soc_device_attribute {
const char *family;
const char *revision;
const char *soc_id;
+
+ const void *data;
};
/**
@@ -34,4 +36,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
*/
struct device *soc_device_to_device(struct soc_device *soc);
+struct soc_device_attribute *soc_device_match(struct soc_device_attribute *matches);
+
#endif /* __SOC_BUS_H */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 1/4] base: soc: introduce soc_device_match() interface
2016-05-30 13:14 ` [PATCH 1/4] base: soc: introduce soc_device_match() interface Arnd Bergmann
@ 2016-05-30 14:57 ` Arnd Bergmann
0 siblings, 0 replies; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 14:57 UTC (permalink / raw)
To: linux-arm-kernel
On Monday, May 30, 2016 3:14:38 PM CEST Arnd Bergmann wrote:
> We keep running into cases where device drivers want to know the exact
> version of the SoC a they are currently running on. In the past, this
> has usually been done through a vendor specific API that can be called
> by a driver, or by directly accessing some kind of version register
> that is not part of the device itself but that belongs to a global
> register area of the chip.
>
> Common reasons for doing this include:
>
> - A machine is not using devicetree or similar for passing data
> about on-chip devices, but just announces their presence using
> boot-time platform devices, and the machine code itself does
> not care about the revision.
>
> - There is existing firmware or boot loaders with existing DT
> binaries with generic compatible strings that do not identify
> the particular revision of each device, but the driver knows
> which SoC revisions include which part
>
> - A prerelease version of a chip has some quirks and we are
> using the same version of the bootloader and the DT blob
> on both the prerelease and the final version. An update of
> the DT binding seems inappropriate because that would involve
> maintaining multiple copies of the dts and/or bootloader.
>
> This introduces the soc_device_match() interface that is meant
> to work like of_match_node() but instead of identifying the
> version of a device, it identifies the SoC itself using a
> vendor-agnostic interface.
>
> Unlike soc_device_match(), we do not do an exact string compare
> but instead use glob_match() to allow wildcards in strings.
I'm sorry the series introduced build failures (I had done some changes
after testing), here is a quick fixup. I'll resend the whole thing
after someone has looked at it as none of the changes below should
have any influence on the review.
Arnd
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index e9623c6674a5..c38573249777 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -173,7 +173,7 @@ module_exit(soc_bus_unregister);
static int soc_device_match_one(struct device *dev, void *arg)
{
struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
- struct soc_device_attribute *match = arg;
+ const struct soc_device_attribute *match = arg;
if (match->machine && !glob_match(match->machine, soc_dev->attr->machine))
return 0;
@@ -208,7 +208,7 @@ static int soc_device_match_one(struct device *dev, void *arg)
* soc_device_attribute to pass a structure or function pointer for
* each entry.
*/
-struct soc_device_attribute *soc_device_match(struct soc_device_attribute *matches)
+const struct soc_device_attribute *soc_device_match(const struct soc_device_attribute *matches)
{
struct device *dev;
int ret;
@@ -219,7 +219,7 @@ struct soc_device_attribute *soc_device_match(struct soc_device_attribute *match
return NULL;
dev = NULL;
- ret = bus_for_each_dev(&soc_bus_type, dev, matches,
+ ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
soc_device_match_one);
}
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 1d4814fe4cb2..a7b8b05a13e8 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -19,6 +19,8 @@
#include <linux/delay.h>
#include <linux/module.h>
#include <linux/mmc/host.h>
+#include <linux/sys_soc.h>
+
#include "sdhci-pltfm.h"
#include "sdhci-esdhc.h"
@@ -75,7 +77,6 @@ static u16 esdhc_readw_fixup(struct sdhci_host *host,
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
- u16 ret;
int shift = (spec_reg & 0x2) * 8;
if (spec_reg == SDHCI_HOST_VERSION)
@@ -565,7 +566,7 @@ static const struct sdhci_pltfm_data sdhci_esdhc_le_pdata = {
};
#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200)
-static const struct soc_device_attribute esdhc_t4240_quirk = {
+static const struct soc_device_attribute esdhc_t4240_quirk[] = {
/* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200 */
{ .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
.data = (void *)(uintptr_t)(T4240_HOST_VER) },
@@ -576,6 +577,7 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host;
struct sdhci_esdhc *esdhc;
+ u32 host_ver;
pltfm_host = sdhci_priv(host);
esdhc = sdhci_pltfm_priv(pltfm_host);
@@ -583,9 +585,9 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
host_ver = sdhci_readw(host, SDHCI_HOST_VERSION);
if (of_device_is_compatible(pdev->dev.of_node, "fsl,t4240-esdhc")) {
- struct soc_device_attribute *match;
+ const struct soc_device_attribute *match;
- match = soc_device_match(&esdhc_t4240_quirk);
+ match = soc_device_match(esdhc_t4240_quirk);
if (match)
host_ver = (uintptr_t)match->data;
}
diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 2f30698f5bcf..476969644bed 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -34,14 +34,6 @@ static u32 fsl_guts_get_svr(struct guts *guts)
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.
@@ -76,13 +68,15 @@ static const struct of_device_id fsl_guts_of_match[] = {
static void fsl_guts_init(struct device *dev, struct guts *guts)
{
- const struct of_device_id *id;
u32 svr = fsl_guts_get_svr(guts);
+ const struct of_device_id *id;
+ const char *socname;
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;
+ socname = id->data;
+ guts->soc.soc_id = devm_kasprintf(dev, GFP_KERNEL, "%s (ver 0x%06x)",
+ socname, svr >> 8);
guts->soc.revision = devm_kasprintf(dev, GFP_KERNEL, "0x%02x",
svr & 0xff);
}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index 02c48c76052b..3dfc8714a88c 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -36,6 +36,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
*/
struct device *soc_device_to_device(struct soc_device *soc);
-struct soc_device_attribute *soc_device_match(struct soc_device_attribute *matches);
+const struct soc_device_attribute *soc_device_match(const struct soc_device_attribute *matches);
#endif /* __SOC_BUS_H */
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-05-26 7:44 ` Ulf Hansson
2016-05-30 13:13 ` Arnd Bergmann
2016-05-30 13:14 ` [PATCH 1/4] base: soc: introduce soc_device_match() interface Arnd Bergmann
@ 2016-05-30 13:15 ` Arnd Bergmann
2016-06-02 1:47 ` Scott Wood
2016-05-30 13:16 ` [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Arnd Bergmann
2016-05-30 13:18 ` [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc" Arnd Bergmann
4 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 13:15 UTC (permalink / raw)
To: linux-arm-kernel
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>
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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-05-30 13:15 ` [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Arnd Bergmann
@ 2016-06-02 1:47 ` Scott Wood
2016-06-02 8:43 ` Arnd Bergmann
0 siblings, 1 reply; 39+ messages in thread
From: Scott Wood @ 2016-06-02 1:47 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-06-02 1:47 ` Scott Wood
@ 2016-06-02 8:43 ` Arnd Bergmann
2016-06-11 1:50 ` Scott Wood
0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-06-02 8:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-06-02 8:43 ` Arnd Bergmann
@ 2016-06-11 1:50 ` Scott Wood
2016-06-23 2:46 ` Yangbo Lu
2016-07-07 2:35 ` Yangbo Lu
0 siblings, 2 replies; 39+ messages in thread
From: Scott Wood @ 2016-06-11 1:50 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-06-11 1:50 ` Scott Wood
@ 2016-06-23 2:46 ` Yangbo Lu
2016-07-07 2:35 ` Yangbo Lu
1 sibling, 0 replies; 39+ messages in thread
From: Yangbo Lu @ 2016-06-23 2:46 UTC (permalink / raw)
To: linux-arm-kernel
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:
> > 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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-06-11 1:50 ` Scott Wood
2016-06-23 2:46 ` Yangbo Lu
@ 2016-07-07 2:35 ` Yangbo Lu
2016-07-07 8:30 ` Arnd Bergmann
1 sibling, 1 reply; 39+ messages in thread
From: Yangbo Lu @ 2016-07-07 2:35 UTC (permalink / raw)
To: linux-arm-kernel
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 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
> 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 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:
> > > 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
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-07-07 2:35 ` Yangbo Lu
@ 2016-07-07 8:30 ` Arnd Bergmann
2016-07-07 20:42 ` Scott Wood
2016-07-08 3:05 ` Yangbo Lu
0 siblings, 2 replies; 39+ messages in thread
From: Arnd Bergmann @ 2016-07-07 8:30 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-07-07 8:30 ` Arnd Bergmann
@ 2016-07-07 20:42 ` Scott Wood
2016-07-08 3:05 ` Yangbo Lu
1 sibling, 0 replies; 39+ messages in thread
From: Scott Wood @ 2016-07-07 20:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
2016-07-07 8:30 ` Arnd Bergmann
2016-07-07 20:42 ` Scott Wood
@ 2016-07-08 3:05 ` Yangbo Lu
1 sibling, 0 replies; 39+ messages in thread
From: Yangbo Lu @ 2016-07-08 3:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: Thursday, July 07, 2016 4:30 PM
> To: Yangbo Lu
> Cc: Scott Wood; linuxppc-dev at lists.ozlabs.org; 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
> 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
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
2016-05-26 7:44 ` Ulf Hansson
` (2 preceding siblings ...)
2016-05-30 13:15 ` [PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms Arnd Bergmann
@ 2016-05-30 13:16 ` Arnd Bergmann
2016-06-02 1:11 ` Scott Wood
2016-05-30 13:18 ` [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc" Arnd Bergmann
4 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 13:16 UTC (permalink / raw)
To: linux-arm-kernel
This is a rewrite of an earlier patch from Yangbo Lu, adding a quirk
for the NXP QorIQ T4240 in the detection of the host device version.
Unfortunately, this device cannot be detected using the compatible
string, as we have to support existing DTS files that use the generic
"fsl,t4240-esdhc" identifier but that have other host versions that
are correctly detected.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 3f34d354f1fc..1d4814fe4cb2 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -73,14 +73,16 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
static u16 esdhc_readw_fixup(struct sdhci_host *host,
int spec_reg, u32 value)
{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
u16 ret;
int shift = (spec_reg & 0x2) * 8;
if (spec_reg == SDHCI_HOST_VERSION)
- ret = value & 0xffff;
- else
- ret = (value >> shift) & 0xffff;
- return ret;
+ return esdhc->vendor_ver << SDHCI_VENDOR_VER_SHIFT |
+ esdhc->spec_ver;
+
+ return (value >> shift) & 0xffff;
}
static u8 esdhc_readb_fixup(struct sdhci_host *host,
@@ -562,16 +564,32 @@ static const struct sdhci_pltfm_data sdhci_esdhc_le_pdata = {
.ops = &sdhci_esdhc_le_ops,
};
+#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200)
+static const struct soc_device_attribute esdhc_t4240_quirk = {
+ /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200 */
+ { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
+ .data = (void *)(uintptr_t)(T4240_HOST_VER) },
+ { },
+};
+
static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host;
struct sdhci_esdhc *esdhc;
- u16 host_ver;
pltfm_host = sdhci_priv(host);
esdhc = sdhci_pltfm_priv(pltfm_host);
host_ver = sdhci_readw(host, SDHCI_HOST_VERSION);
+
+ if (of_device_is_compatible(pdev->dev.of_node, "fsl,t4240-esdhc")) {
+ struct soc_device_attribute *match;
+
+ match = soc_device_match(&esdhc_t4240_quirk);
+ if (match)
+ host_ver = (uintptr_t)match->data;
+ }
+
esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
SDHCI_VENDOR_VER_SHIFT;
esdhc->spec_ver = host_ver & SDHCI_SPEC_VER_MASK;
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
2016-05-30 13:16 ` [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Arnd Bergmann
@ 2016-06-02 1:11 ` Scott Wood
2016-06-02 8:52 ` Arnd Bergmann
0 siblings, 1 reply; 39+ messages in thread
From: Scott Wood @ 2016-06-02 1:11 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 2016-05-30 at 15:16 +0200, Arnd Bergmann wrote:
> This is a rewrite of an earlier patch from Yangbo Lu, adding a quirk
> for the NXP QorIQ T4240 in the detection of the host device version.
>
> Unfortunately, this device cannot be detected using the compatible
> string, as we have to support existing DTS files that use the generic
> "fsl,t4240-esdhc" identifier but that have other host versions that
> are correctly detected.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of
> -esdhc.c
> index 3f34d354f1fc..1d4814fe4cb2 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -73,14 +73,16 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
> static u16 esdhc_readw_fixup(struct sdhci_host *host,
> int spec_reg, u32 value)
> {
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
> u16 ret;
> int shift = (spec_reg & 0x2) * 8;
>
> if (spec_reg == SDHCI_HOST_VERSION)
> - ret = value & 0xffff;
> - else
> - ret = (value >> shift) & 0xffff;
> - return ret;
> + return esdhc->vendor_ver << SDHCI_VENDOR_VER_SHIFT |
> + esdhc->spec_ver;
> +
> + return (value >> shift) & 0xffff;
> }
>
> static u8 esdhc_readb_fixup(struct sdhci_host *host,
> @@ -562,16 +564,32 @@ static const struct sdhci_pltfm_data
> sdhci_esdhc_le_pdata = {
> .ops = &sdhci_esdhc_le_ops,
> };
>
> +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> SDHCI_SPEC_200)
> +static const struct soc_device_attribute esdhc_t4240_quirk = {
> + /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> */
> + { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> + .data = (void *)(uintptr_t)(T4240_HOST_VER) },
Why should this code need to care that the string begins with "T4"? This
creates dual maintenance if that were to change. It's also broken because
T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
-2.0" and thus with these patches it would incorrectly show up as "P series
(0x824000)". The compatible string of this node was never meant to be a key
for choosing a string to describe the system to userspace.
0x824000 is a magic number which should be represented symbolically.
If T4240 is affected, then so are the reduced-core variants T4160 and T4080,
but 0x824000 doesn't match them (Yangbo's patch had the same problem). And
please don't respond with "0x824*"
You also didn't strip out the E bit of SVR which indicates encryption
capability and nothing else (Yangbo's patch did not have this problem because
it used SVR_SOC_VER).
What happens if the revision condition is more complicated, such as <= 0x20
with 0x21 being fine? Multiple quirk entries where before we had as simple
comparison?
I fail to see how this approach is an improvement (much less one that needs to
hold up a patchset that is fixing a problem and is not touching any generic
code). Why does this need to be a string?
-Scott
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
2016-06-02 1:11 ` Scott Wood
@ 2016-06-02 8:52 ` Arnd Bergmann
2016-06-11 2:00 ` Scott Wood
0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-06-02 8:52 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, June 1, 2016 8:11:14 PM CEST Scott Wood wrote:
> > +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> > SDHCI_SPEC_200)
> > +static const struct soc_device_attribute esdhc_t4240_quirk = {
> > + /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> > */
> > + { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> > + .data = (void *)(uintptr_t)(T4240_HOST_VER) },
>
> Why should this code need to care that the string begins with "T4"? This
> creates dual maintenance if that were to change. It's also broken because
> T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
> -2.0" and thus with these patches it would incorrectly show up as "P series
> (0x824000)". The compatible string of this node was never meant to be a key
> for choosing a string to describe the system to userspace.
This is an artifact of not knowing the specific SoC name, and we can change
that by looking up the name from the SVR value in the soc_device driver.
> 0x824000 is a magic number which should be represented symbolically.
Sure, feel free to change the format of the soc_device string in any
name, I just converted the information I had available in the comments.
The only thing that is important here is that the string matches what
the soc_device driver calls it.
> If T4240 is affected, then so are the reduced-core variants T4160 and T4080,
> but 0x824000 doesn't match them (Yangbo's patch had the same problem). And
> please don't respond with "0x824*"
>
> You also didn't strip out the E bit of SVR which indicates encryption
> capability and nothing else (Yangbo's patch did not have this problem because
> it used SVR_SOC_VER).
Ok, that should be easy enough to fix in the soc_device driver.
> What happens if the revision condition is more complicated, such as <= 0x20
> with 0x21 being fine? Multiple quirk entries where before we had as simple
> comparison?
I guess yes. I would really hope that there is no need to use this interface
pervasively, it's really just to work around the cases where there is no
way to pass the information in DT otherwise.
> I fail to see how this approach is an improvement (much less one that needs to
> hold up a patchset that is fixing a problem and is not touching any generic
> code). Why does this need to be a string?
A string is what user space gets in /sys/devices/soc/*, and we already have
code that does the same things there to work around quirks, here we just
use the same interface in a completely generic way. Note that not every
SoC family uses numbers in the same way, some have multiple subrevisions,
some have names etc.
Arnd
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
2016-06-02 8:52 ` Arnd Bergmann
@ 2016-06-11 2:00 ` Scott Wood
0 siblings, 0 replies; 39+ messages in thread
From: Scott Wood @ 2016-06-11 2:00 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2016-06-02 at 10:52 +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 8:11:14 PM CEST Scott Wood wrote:
> > > +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> > > SDHCI_SPEC_200)
> > > +static const struct soc_device_attribute esdhc_t4240_quirk = {
> > > + /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> > > */
> > > + { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> > > + .data = (void *)(uintptr_t)(T4240_HOST_VER) },
> >
> > Why should this code need to care that the string begins with "T4"? This
> > creates dual maintenance if that were to change. It's also broken because
> > T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
> > -2.0" and thus with these patches it would incorrectly show up as "P
> > series
> > (0x824000)". The compatible string of this node was never meant to be a
> > key
> > for choosing a string to describe the system to userspace.
>
> This is an artifact of not knowing the specific SoC name, and we can change
> that by looking up the name from the SVR value in the soc_device driver.
...or we could keep it simple and just match the number.
> > 0x824000 is a magic number which should be represented symbolically.
>
> Sure, feel free to change the format of the soc_device string in any
> name,
That's not what I was asking for... The match should be numeric but the
knowledge of what the number is should come from a symbolic #define.
> > If T4240 is affected, then so are the reduced-core variants T4160 and
> > T4080,
> > but 0x824000 doesn't match them (Yangbo's patch had the same problem).
> > And
> > please don't respond with "0x824*"
> >
> > You also didn't strip out the E bit of SVR which indicates encryption
> > capability and nothing else (Yangbo's patch did not have this problem
> > because
> > it used SVR_SOC_VER).
>
> Ok, that should be easy enough to fix in the soc_device driver.
No, because the soc_device driver doesn't know whether the consumer of the ID
cares about the E bit.
> > What happens if the revision condition is more complicated, such as <=
> > 0x20
> > with 0x21 being fine? Multiple quirk entries where before we had as
> > simple
> > comparison?
>
> I guess yes. I would really hope that there is no need to use this interface
> pervasively, it's really just to work around the cases where there is no
> way to pass the information in DT otherwise.
How does putting it in the DT work when you have multiple versions of the same
SoC, some of which have the bug and some which don't?
> > I fail to see how this approach is an improvement (much less one that
> > needs to
> > hold up a patchset that is fixing a problem and is not touching any
> > generic
> > code). Why does this need to be a string?
>
> A string is what user space gets in /sys/devices/soc/*,
It is rare that the kernel accesses information in the exact same way that
userspace does. And once we expose this to userspace we're stuck with it, so
exporting anything other than a simple number is even less desirable.
> and we already have
> code that does the same things there to work around quirks, here we just
> use the same interface in a completely generic way. Note that not every
> SoC family uses numbers in the same way, some have multiple subrevisions,
> some have names etc.
Where is the need for a "completely generic way" for one piece of vendor
-specific code to get information that is inherently specific to that vendor,
that is supplied by code specific to that vendor?
-Scott
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"
2016-05-26 7:44 ` Ulf Hansson
` (3 preceding siblings ...)
2016-05-30 13:16 ` [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Arnd Bergmann
@ 2016-05-30 13:18 ` Arnd Bergmann
2016-06-02 1:24 ` Scott Wood
4 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-05-30 13:18 UTC (permalink / raw)
To: linux-arm-kernel
All users of this driver are PowerPC specific and the header file
has no business in the global include/linux/ hierarchy, so move
it back before anyone starts using it on ARM.
This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This part of the series is not required for the eSDHC quirk,
but it restores the asm/fsl_guts.h header so it doesn't accidentally
get abused for this in the future. I found two drivers outside of
arch/powerpc that already accessed the registers directly, but the
functions look fairly contained, and can be easily hidden in an
#ifdef CONFIG_PPC
diff --git a/include/linux/fsl/guts.h b/arch/powerpc/include/asm/fsl_guts.h
similarity index 99%
rename from include/linux/fsl/guts.h
rename to arch/powerpc/include/asm/fsl_guts.h
index 649e9171a9b3..a67413c52701 100644
--- a/include/linux/fsl/guts.h
+++ b/arch/powerpc/include/asm/fsl_guts.h
@@ -12,10 +12,9 @@
* option) any later version.
*/
-#ifndef __FSL_GUTS_H__
-#define __FSL_GUTS_H__
-
-#include <linux/types.h>
+#ifndef __ASM_POWERPC_FSL_GUTS_H__
+#define __ASM_POWERPC_FSL_GUTS_H__
+#ifdef __KERNEL__
/**
* Global Utility Registers.
@@ -295,3 +294,4 @@ struct ccsr_rcpm_v2 {
};
#endif
+#endif
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index f61cbe235581..00f052e9f2a2 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -34,7 +34,6 @@
#include <linux/of_device.h>
#include <linux/phy.h>
#include <linux/memblock.h>
-#include <linux/fsl/guts.h>
#include <linux/atomic.h>
#include <asm/time.h>
@@ -52,6 +51,7 @@
#include <soc/fsl/qe/qe_ic.h>
#include <asm/mpic.h>
#include <asm/swiotlb.h>
+#include <asm/fsl_guts.h>
#include "smp.h"
#include "mpc85xx.h"
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c b/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c
index f05325f0cc03..a812c0511252 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_pm_ops.c
@@ -14,8 +14,8 @@
#include <linux/kernel.h>
#include <linux/of.h>
#include <linux/of_address.h>
-#include <linux/fsl/guts.h>
+#include <asm/fsl_guts.h>
#include <asm/io.h>
#include <asm/fsl_pm.h>
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
index 3f4dad133338..453ddda00fce 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
@@ -17,7 +17,6 @@
#include <linux/seq_file.h>
#include <linux/interrupt.h>
#include <linux/of_platform.h>
-#include <linux/fsl/guts.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -28,6 +27,7 @@
#include <asm/mpic.h>
#include <soc/fsl/qe/qe.h>
#include <soc/fsl/qe/qe_ic.h>
+#include <asm/fsl_guts.h>
#include <sysdev/fsl_soc.h>
#include <sysdev/fsl_pci.h>
diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index 371df822e88e..6ac986d3f8a3 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -16,7 +16,6 @@
* kind, whether express or implied.
*/
-#include <linux/fsl/guts.h>
#include <linux/pci.h>
#include <linux/of_platform.h>
#include <asm/div64.h>
@@ -26,6 +25,7 @@
#include <sysdev/fsl_soc.h>
#include <sysdev/fsl_pci.h>
#include <asm/udbg.h>
+#include <asm/fsl_guts.h>
#include <asm/fsl_lbc.h>
#include "smp.h"
diff --git a/arch/powerpc/platforms/85xx/p1022_rdk.c b/arch/powerpc/platforms/85xx/p1022_rdk.c
index 5087becaa8bc..680232d6ba48 100644
--- a/arch/powerpc/platforms/85xx/p1022_rdk.c
+++ b/arch/powerpc/platforms/85xx/p1022_rdk.c
@@ -12,7 +12,6 @@
* kind, whether express or implied.
*/
-#include <linux/fsl/guts.h>
#include <linux/pci.h>
#include <linux/of_platform.h>
#include <asm/div64.h>
@@ -22,6 +21,7 @@
#include <sysdev/fsl_soc.h>
#include <sysdev/fsl_pci.h>
#include <asm/udbg.h>
+#include <asm/fsl_guts.h>
#include "smp.h"
#include "mpc85xx.h"
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index fe9f19e5e935..6bd3a292e790 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -18,7 +18,6 @@
#include <linux/kexec.h>
#include <linux/highmem.h>
#include <linux/cpu.h>
-#include <linux/fsl/guts.h>
#include <asm/machdep.h>
#include <asm/pgtable.h>
@@ -26,6 +25,7 @@
#include <asm/mpic.h>
#include <asm/cacheflush.h>
#include <asm/dbell.h>
+#include <asm/fsl_guts.h>
#include <asm/code-patching.h>
#include <asm/cputhreads.h>
#include <asm/fsl_pm.h>
diff --git a/arch/powerpc/platforms/85xx/twr_p102x.c b/arch/powerpc/platforms/85xx/twr_p102x.c
index 71bc255b4324..2cac45f72d24 100644
--- a/arch/powerpc/platforms/85xx/twr_p102x.c
+++ b/arch/powerpc/platforms/85xx/twr_p102x.c
@@ -15,7 +15,6 @@
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/errno.h>
-#include <linux/fsl/guts.h>
#include <linux/pci.h>
#include <linux/of_platform.h>
@@ -24,6 +23,7 @@
#include <asm/mpic.h>
#include <soc/fsl/qe/qe.h>
#include <soc/fsl/qe/qe_ic.h>
+#include <asm/fsl_guts.h>
#include <sysdev/fsl_soc.h>
#include <sysdev/fsl_pci.h>
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
index 957473e5c8e5..761c81476957 100644
--- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
+++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
@@ -24,7 +24,6 @@
#include <linux/delay.h>
#include <linux/seq_file.h>
#include <linux/of.h>
-#include <linux/fsl/guts.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -39,6 +38,7 @@
#include <sysdev/fsl_pci.h>
#include <sysdev/fsl_soc.h>
#include <sysdev/simple_gpio.h>
+#include <asm/fsl_guts.h>
#include "mpc86xx.h"
diff --git a/arch/powerpc/sysdev/fsl_rcpm.c b/arch/powerpc/sysdev/fsl_rcpm.c
index 9259a94f70e1..8af22187cb25 100644
--- a/arch/powerpc/sysdev/fsl_rcpm.c
+++ b/arch/powerpc/sysdev/fsl_rcpm.c
@@ -19,7 +19,7 @@
#include <linux/export.h>
#include <asm/io.h>
-#include <linux/fsl/guts.h>
+#include <asm/fsl_guts.h>
#include <asm/cputhreads.h>
#include <asm/fsl_pm.h>
#include <asm/smp.h>
diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index 58566a17944a..f311bd399672 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -12,7 +12,6 @@
#include <linux/clk.h>
#include <linux/clk-provider.h>
-#include <linux/fsl/guts.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -21,6 +20,10 @@
#include <linux/of.h>
#include <linux/slab.h>
+#ifdef CONFIG_PPC
+#include <asm/fsl_guts.h>
+#endif
+
#define PLL_DIV1 0
#define PLL_DIV2 1
#define PLL_DIV3 2
@@ -341,6 +344,8 @@ static const struct clockgen_muxinfo t4240_hwa5 = {
},
};
+#ifdef CONFIG_PPC
+
#define RCWSR7_FM1_CLK_SEL 0x40000000
#define RCWSR7_FM2_CLK_SEL 0x20000000
#define RCWSR7_HWA_ASYNC_DIV 0x04000000
@@ -408,6 +413,7 @@ static void __init p5040_init_periph(struct clockgen *cg)
else
cg->fman[1] = cg->pll[PLATFORM_PLL].div[PLL_DIV2].clk;
}
+#endif
static void __init t1023_init_periph(struct clockgen *cg)
{
@@ -499,6 +505,7 @@ static const struct clockgen_chipinfo chipinfo[] = {
.pll_mask = 0x37,
.flags = CG_VER3 | CG_LITTLE_ENDIAN,
},
+#ifdef CONFIG_PPC
{
.compat = "fsl,p2041-clockgen",
.guts_compat = "fsl,qoriq-device-config-1.0",
@@ -559,6 +566,7 @@ static const struct clockgen_chipinfo chipinfo[] = {
},
.pll_mask = 0x0f,
},
+#endif
{
.compat = "fsl,t1023-clockgen",
.guts_compat = "fsl,t1023-device-config",
diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
index a34355fca37a..2570f2a25dc4 100644
--- a/drivers/iommu/fsl_pamu.c
+++ b/drivers/iommu/fsl_pamu.c
@@ -20,11 +20,11 @@
#include "fsl_pamu.h"
-#include <linux/fsl/guts.h>
#include <linux/interrupt.h>
#include <linux/genalloc.h>
#include <asm/mpc85xx.h>
+#include <asm/fsl_guts.h>
/* define indexes for each operation mapping scenario */
#define OMI_QMAN 0x00
diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 1de2e1e51c2b..dabee93fcadd 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -35,7 +35,6 @@
#include "fman.h"
#include "fman_muram.h"
-#include <linux/fsl/guts.h>
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/module.h>
@@ -46,6 +45,10 @@
#include <linux/interrupt.h>
#include <linux/libfdt_env.h>
+#ifdef CONFIG_PPC
+#include <asm/fsl_guts.h>
+#endif
+
/* General defines */
#define FMAN_LIODN_TBL 64 /* size of LIODN table */
#define MAX_NUM_OF_MACS 10
@@ -1889,7 +1892,9 @@ static int fman_reset(struct fman *fman)
err = -EBUSY;
goto _return;
- } else {
+ }
+#ifdef CONFIG_PPC
+ else {
struct device_node *guts_node;
struct ccsr_guts __iomem *guts_regs;
u32 devdisr2, reg;
@@ -1952,6 +1957,7 @@ guts_node:
dev_dbg(fman->dev, "%s: Didn't perform FManV3 reset due to Errata A007273!\n",
__func__);
}
+#endif
_return:
return err;
}
diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c
index ddf49f30b23f..73b2b89d4a3e 100644
--- a/sound/soc/fsl/mpc8610_hpcd.c
+++ b/sound/soc/fsl/mpc8610_hpcd.c
@@ -12,11 +12,11 @@
#include <linux/module.h>
#include <linux/interrupt.h>
-#include <linux/fsl/guts.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/slab.h>
#include <sound/soc.h>
+#include <asm/fsl_guts.h>
#include "fsl_dma.h"
#include "fsl_ssi.h"
diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c
index a1f780ecadf5..a24e04919f71 100644
--- a/sound/soc/fsl/p1022_ds.c
+++ b/sound/soc/fsl/p1022_ds.c
@@ -11,12 +11,12 @@
*/
#include <linux/module.h>
-#include <linux/fsl/guts.h>
#include <linux/interrupt.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/slab.h>
#include <sound/soc.h>
+#include <asm/fsl_guts.h>
#include "fsl_dma.h"
#include "fsl_ssi.h"
diff --git a/sound/soc/fsl/p1022_rdk.c b/sound/soc/fsl/p1022_rdk.c
index d4d88a8cb9c0..d8448887bfda 100644
--- a/sound/soc/fsl/p1022_rdk.c
+++ b/sound/soc/fsl/p1022_rdk.c
@@ -18,12 +18,12 @@
*/
#include <linux/module.h>
-#include <linux/fsl/guts.h>
#include <linux/interrupt.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/slab.h>
#include <sound/soc.h>
+#include <asm/fsl_guts.h>
#include "fsl_dma.h"
#include "fsl_ssi.h"
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"
2016-05-30 13:18 ` [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc" Arnd Bergmann
@ 2016-06-02 1:24 ` Scott Wood
2016-06-02 9:01 ` Arnd Bergmann
0 siblings, 1 reply; 39+ messages in thread
From: Scott Wood @ 2016-06-02 1:24 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> All users of this driver are PowerPC specific and the header file
> has no business in the global include/linux/ hierarchy, so move
> it back before anyone starts using it on ARM.
>
> This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This part of the series is not required for the eSDHC quirk,
> but it restores the asm/fsl_guts.h header so it doesn't accidentally
> get abused for this in the future. I found two drivers outside of
> arch/powerpc that already accessed the registers directly, but the
> functions look fairly contained, and can be easily hidden in an
> #ifdef CONFIG_PPC
NACK
Besides adding ifdef pollution for no good reason, this register block is used
on some ARM chips as well. Why is it a problem if "anyone starts using it on
ARM"?
BTW, of all the mailing lists you included on this CC, you seem to have left
off the PPC list (I've added it).
-Scott
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"
2016-06-02 1:24 ` Scott Wood
@ 2016-06-02 9:01 ` Arnd Bergmann
2016-06-11 2:13 ` Scott Wood
0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2016-06-02 9:01 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, June 1, 2016 8:24:20 PM CEST Scott Wood wrote:
> On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> > All users of this driver are PowerPC specific and the header file
> > has no business in the global include/linux/ hierarchy, so move
> > it back before anyone starts using it on ARM.
> >
> > This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > This part of the series is not required for the eSDHC quirk,
> > but it restores the asm/fsl_guts.h header so it doesn't accidentally
> > get abused for this in the future. I found two drivers outside of
> > arch/powerpc that already accessed the registers directly, but the
> > functions look fairly contained, and can be easily hidden in an
> > #ifdef CONFIG_PPC
>
> NACK
>
> Besides adding ifdef pollution for no good reason, this register block is used
> on some ARM chips as well. Why is it a problem if "anyone starts using it on
> ARM"?
It's just not a good interface when it's defined as "this is the layout of
a register area that any driver can ioremap() if they can figure out the
device node". It's not uncommon to have register areas like that, but
normally you have at the minimum a 'syscon' device to handle locking
between drivers accessing the same registers and to avoid having to map
the same area multiple times.
If we need to use 'guts' registers on ARM, we can find a way to abstract
them properly for the given use cases, using a syscon or a driver with
exported functions, but just making a PowerPC platform specific header
global to all Linux drivers by putting it into include/linux doesn't seem
right.
Note that the header file uses a structure definition rather than the more
common macros with register offsets, which is fine for a driver that has
its own registers and abstracts them, but it doesn't really work with
the regmap interface, so if we want to use it with syscon, it also needs to
be rewritten.
> BTW, of all the mailing lists you included on this CC, you seem to have left
> off the PPC list (I've added it).
Sorry, my mistake.
Arnd
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"
2016-06-02 9:01 ` Arnd Bergmann
@ 2016-06-11 2:13 ` Scott Wood
0 siblings, 0 replies; 39+ messages in thread
From: Scott Wood @ 2016-06-11 2:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2016-06-02 at 11:01 +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 8:24:20 PM CEST Scott Wood wrote:
> > On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> > > All users of this driver are PowerPC specific and the header file
> > > has no business in the global include/linux/ hierarchy, so move
> > > it back before anyone starts using it on ARM.
> > >
> > > This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
> > >
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > > This part of the series is not required for the eSDHC quirk,
> > > but it restores the asm/fsl_guts.h header so it doesn't accidentally
> > > get abused for this in the future. I found two drivers outside of
> > > arch/powerpc that already accessed the registers directly, but the
> > > functions look fairly contained, and can be easily hidden in an
> > > #ifdef CONFIG_PPC
> >
> > NACK
> >
> > Besides adding ifdef pollution for no good reason, this register block is
> > used
> > on some ARM chips as well. Why is it a problem if "anyone starts using it
> > on
> > ARM"?
>
> It's just not a good interface when it's defined as "this is the layout of
> a register area that any driver can ioremap() if they can figure out the
> device node".
That's why I want to move accesses into one guts driver.
> It's not uncommon to have register areas like that, but
> normally you have at the minimum a 'syscon' device to handle locking
> between drivers accessing the same registers and to avoid having to map
> the same area multiple times.
syscon requires device tree changes.
I don't see read-modify-write operations in regmap -- how does locking around
an individual, inherently-atomic load or store help?
> If we need to use 'guts' registers on ARM, we can find a way to abstract
> them properly for the given use cases, using a syscon or a driver with
> exported functions, but just making a PowerPC platform specific header
> global to all Linux drivers by putting it into include/linux doesn't seem
> right.
Again, it's not PowerPC-specific! It started that way but then the same
register block got put onto some ARM chips.
It's not global to "all Linux drivers", just the ones that choose to include
an fsl-specific header.
If and when all uses of guts are moved into the guts driver, the header can be
moved into drivers/soc/fsl.
> Note that the header file uses a structure definition rather than the more
> common macros with register offsets, which is fine for a driver that has
> its own registers and abstracts them, but it doesn't really work with
> the regmap interface, so if we want to use it with syscon, it also needs to
> be rewritten.
We don't want to use it with syscon. If we did, the solution wouldn't be to
move the header back to arch/powerpc, but to convert the struct into offsets.
-Scott
^ permalink raw reply [flat|nested] 39+ messages in thread