[PATCH v2 0/3] Rockchip: generalize GRF setup

[PATCH v2 0/3] Rockchip: generalize GRF setup

[Heiko Stuebner]

The General register files contain a big bunch of settings for various
components. Things like the automatic sdmmc/jtag switch may even affect
us in a bad way, while that property (and possibly others) are not even
part of the dw_mmc controller itself.

And while the rk3288 could still carry adaptions to these defaults
in its mach files, this is no longer possible on the arm64 socs, while
things like the jtag-switch from above still can affect us there.

changes in v2:
- address comments from Doug Anderson
- drop platform device, as we actual probing may happen at some
  unspecified later time during boot
- add reviews / acks received in v1

Heiko Stuebner (3):
  dt-bindings: add used but undocumented rockchip grf compatible values
  soc: rockchip: add driver handling grf setup
  ARM: rockchip: drop rk3288 jtag/mmc switch handling

 .../devicetree/bindings/soc/rockchip/grf.txt       |   4 +
 arch/arm/mach-rockchip/rockchip.c                  |  12 --
 drivers/soc/rockchip/Kconfig                       |  10 ++
 drivers/soc/rockchip/Makefile                      |   1 +
 drivers/soc/rockchip/grf.c                         | 134 +++++++++++++++++++++
 5 files changed, 149 insertions(+), 12 deletions(-)
 create mode 100644 drivers/soc/rockchip/grf.c

-- 
2.10.2

[PATCH v2 0/3] Rockchip: generalize GRF setup

[Heiko Stuebner]

The General register files contain a big bunch of settings for various
components. Things like the automatic sdmmc/jtag switch may even affect
us in a bad way, while that property (and possibly others) are not even
part of the dw_mmc controller itself.

And while the rk3288 could still carry adaptions to these defaults
in its mach files, this is no longer possible on the arm64 socs, while
things like the jtag-switch from above still can affect us there.

changes in v2:
- address comments from Doug Anderson
- drop platform device, as we actual probing may happen at some
  unspecified later time during boot
- add reviews / acks received in v1

Heiko Stuebner (3):
  dt-bindings: add used but undocumented rockchip grf compatible values
  soc: rockchip: add driver handling grf setup
  ARM: rockchip: drop rk3288 jtag/mmc switch handling

 .../devicetree/bindings/soc/rockchip/grf.txt       |   4 +
 arch/arm/mach-rockchip/rockchip.c                  |  12 --
 drivers/soc/rockchip/Kconfig                       |  10 ++
 drivers/soc/rockchip/Makefile                      |   1 +
 drivers/soc/rockchip/grf.c                         | 134 +++++++++++++++++++++
 5 files changed, 149 insertions(+), 12 deletions(-)
 create mode 100644 drivers/soc/rockchip/grf.c

-- 
2.10.2

[PATCH v2 1/3] dt-bindings: add used but undocumented rockchip grf compatible values

[Heiko Stuebner]

There are some more General Register Files used in devicetree files
already, but as of now undocumented in the binding document, fix that.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 Documentation/devicetree/bindings/soc/rockchip/grf.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.txt b/Documentation/devicetree/bindings/soc/rockchip/grf.txt
index 013e71a..c6e62cb 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.txt
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.txt
@@ -5,11 +5,13 @@ is composed of many registers for system control.
 
 From RK3368 SoCs, the GRF is divided into two sections,
 - GRF, used for general non-secure system,
+- SGRF, used for general secure system,
 - PMUGRF, used for always on system
 
 Required Properties:
 
 - compatible: GRF should be one of the followings
+   - "rockchip,rk3036-grf", "syscon": for rk3036
    - "rockchip,rk3066-grf", "syscon": for rk3066
    - "rockchip,rk3188-grf", "syscon": for rk3188
    - "rockchip,rk3228-grf", "syscon": for rk3228
@@ -19,6 +21,8 @@ Required Properties:
 - compatible: PMUGRF should be one of the followings
    - "rockchip,rk3368-pmugrf", "syscon": for rk3368
    - "rockchip,rk3399-pmugrf", "syscon": for rk3399
+- compatible: SGRF should be one of the following
+   - "rockchip,rk3288-sgrf", "syscon": for rk3288
 - reg: physical base address of the controller and length of memory mapped
   region.
 
-- 
2.10.2

[PATCH v2 1/3] dt-bindings: add used but undocumented rockchip grf compatible values

[Heiko Stuebner]

There are some more General Register Files used in devicetree files
already, but as of now undocumented in the binding document, fix that.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 Documentation/devicetree/bindings/soc/rockchip/grf.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.txt b/Documentation/devicetree/bindings/soc/rockchip/grf.txt
index 013e71a..c6e62cb 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.txt
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.txt
@@ -5,11 +5,13 @@ is composed of many registers for system control.
 
 From RK3368 SoCs, the GRF is divided into two sections,
 - GRF, used for general non-secure system,
+- SGRF, used for general secure system,
 - PMUGRF, used for always on system
 
 Required Properties:
 
 - compatible: GRF should be one of the followings
+   - "rockchip,rk3036-grf", "syscon": for rk3036
    - "rockchip,rk3066-grf", "syscon": for rk3066
    - "rockchip,rk3188-grf", "syscon": for rk3188
    - "rockchip,rk3228-grf", "syscon": for rk3228
@@ -19,6 +21,8 @@ Required Properties:
 - compatible: PMUGRF should be one of the followings
    - "rockchip,rk3368-pmugrf", "syscon": for rk3368
    - "rockchip,rk3399-pmugrf", "syscon": for rk3399
+- compatible: SGRF should be one of the following
+   - "rockchip,rk3288-sgrf", "syscon": for rk3288
 - reg: physical base address of the controller and length of memory mapped
   region.
 
-- 
2.10.2

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

The General Register Files are an area of registers containing a lot
of single-bit settings for numerous components as well full components
like usbphy control. Therefore all used components are accessed
via the syscon provided by the grf nodes or from the sub-devices
created through the simple-mfd created from the grf node.

Some settings are not used by anything but will need to be set up
according to expectations on the kernel side.

Best example is the force_jtag setting, which defaults to on and
results in the soc switching the pin-outputs between jtag and sdmmc
automatically depending on the card-detect status. This conflicts
heavily with how the dw_mmc driver expects to do its work and also
with the clock-controller, which has most likely deactivated the
jtag clock due to it being unused.

So far the handling of this setting was living in the mach-rockchip
code for the arm32-based rk3288 but that of course doesn't work
for arm64 socs and would also look ugly for further arm32 socs.

Also always disabling this setting is quite specific to linux and
its subsystems, other operating systems might prefer other settings,
so that the bootloader cannot really set a sane default for all.

So introduce a top-level driver for the grf that handles these
settings that need to be a certain way but nobody cares about.

Other needed settings might surface in the future and can then
be added here, but only as a last option. Ideally general GRF
settings should be handled in the driver needing them.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/soc/rockchip/Kconfig  |  10 ++++
 drivers/soc/rockchip/Makefile |   1 +
 drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 drivers/soc/rockchip/grf.c

diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
index 7140ff8..20da55d 100644
--- a/drivers/soc/rockchip/Kconfig
+++ b/drivers/soc/rockchip/Kconfig
@@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
 #
 # Rockchip Soc drivers
 #
+
+config ROCKCHIP_GRF
+	bool
+	default y
+	help
+	  The General Register Files are a central component providing
+	  special additional settings registers for a lot of soc-components.
+	  In a lot of cases there also need to be default settings initialized
+	  to make some of them conform to expectations of the kernel.
+
 config ROCKCHIP_PM_DOMAINS
         bool "Rockchip generic power domain"
         depends on PM
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
index 3d73d06..c851fa0 100644
--- a/drivers/soc/rockchip/Makefile
+++ b/drivers/soc/rockchip/Makefile
@@ -1,4 +1,5 @@
 #
 # Rockchip Soc drivers
 #
+obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
 obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
new file mode 100644
index 0000000..0c85476a
--- /dev/null
+++ b/drivers/soc/rockchip/grf.c
@@ -0,0 +1,134 @@
+/*
+ * Rockchip Generic Register Files setup
+ *
+ * Copyright (c) 2016 Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
+
+struct rockchip_grf_value {
+	const char *desc;
+	u32 reg;
+	u32 val;
+};
+
+struct rockchip_grf_info {
+	const struct rockchip_grf_value *values;
+	int num_values;
+};
+
+#define RK3036_GRF_SOC_CON0		0x140
+
+static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
+	/*
+	 * Disable auto jtag/sdmmc switching that causes issues with the
+	 * clock-framework and the mmc controllers making them unreliable.
+	 */
+	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
+};
+
+static const struct rockchip_grf_info rk3036_grf __initconst = {
+	.values = rk3036_defaults,
+	.num_values = ARRAY_SIZE(rk3036_defaults),
+};
+
+#define RK3288_GRF_SOC_CON0		0x244
+
+static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
+	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
+};
+
+static const struct rockchip_grf_info rk3288_grf __initconst = {
+	.values = rk3288_defaults,
+	.num_values = ARRAY_SIZE(rk3288_defaults),
+};
+
+#define RK3368_GRF_SOC_CON15		0x43c
+
+static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
+	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
+};
+
+static const struct rockchip_grf_info rk3368_grf __initconst = {
+	.values = rk3368_defaults,
+	.num_values = ARRAY_SIZE(rk3368_defaults),
+};
+
+#define RK3399_GRF_SOC_CON7		0xe21c
+
+static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
+	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
+};
+
+static const struct rockchip_grf_info rk3399_grf __initconst = {
+	.values = rk3399_defaults,
+	.num_values = ARRAY_SIZE(rk3399_defaults),
+};
+
+static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
+	{
+		.compatible = "rockchip,rk3036-grf",
+		.data = (void *)&rk3036_grf,
+	}, {
+		.compatible = "rockchip,rk3288-grf",
+		.data = (void *)&rk3288_grf,
+	}, {
+		.compatible = "rockchip,rk3368-grf",
+		.data = (void *)&rk3368_grf,
+	}, {
+		.compatible = "rockchip,rk3399-grf",
+		.data = (void *)&rk3399_grf,
+	},
+	{ /* sentinel */ },
+};
+
+static int __init rockchip_grf_init(void)
+{
+	const struct rockchip_grf_info *grf_info;
+	const struct of_device_id *match;
+	struct device_node *np;
+	struct regmap *grf;
+	int ret, i;
+
+	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match, &match);
+	if (!np)
+		return -ENODEV;
+	if (!match || !match->data) {
+		pr_err("%s: missing grf data\n", __func__);
+		return -EINVAL;
+	}
+
+	grf_info = match->data;
+
+	grf = syscon_node_to_regmap(np);
+	if (IS_ERR(grf)) {
+		pr_err("%s: could not get grf syscon\n", __func__);
+		return PTR_ERR(grf);
+	}
+
+	for (i = 0; i < grf_info->num_values; i++) {
+		const struct rockchip_grf_value *val = &grf_info->values[i];
+
+		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
+			val->desc, val->reg, val->val);
+		ret = regmap_write(grf, val->reg, val->val);
+		if (ret < 0)
+			pr_err("%s: write to %#6x failed with %d\n",
+			       __func__, val->reg, ret);
+	}
+
+	return 0;
+}
+postcore_initcall(rockchip_grf_init);
-- 
2.10.2

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

The General Register Files are an area of registers containing a lot
of single-bit settings for numerous components as well full components
like usbphy control. Therefore all used components are accessed
via the syscon provided by the grf nodes or from the sub-devices
created through the simple-mfd created from the grf node.

Some settings are not used by anything but will need to be set up
according to expectations on the kernel side.

Best example is the force_jtag setting, which defaults to on and
results in the soc switching the pin-outputs between jtag and sdmmc
automatically depending on the card-detect status. This conflicts
heavily with how the dw_mmc driver expects to do its work and also
with the clock-controller, which has most likely deactivated the
jtag clock due to it being unused.

So far the handling of this setting was living in the mach-rockchip
code for the arm32-based rk3288 but that of course doesn't work
for arm64 socs and would also look ugly for further arm32 socs.

Also always disabling this setting is quite specific to linux and
its subsystems, other operating systems might prefer other settings,
so that the bootloader cannot really set a sane default for all.

So introduce a top-level driver for the grf that handles these
settings that need to be a certain way but nobody cares about.

Other needed settings might surface in the future and can then
be added here, but only as a last option. Ideally general GRF
settings should be handled in the driver needing them.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/soc/rockchip/Kconfig  |  10 ++++
 drivers/soc/rockchip/Makefile |   1 +
 drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 drivers/soc/rockchip/grf.c

diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
index 7140ff8..20da55d 100644
--- a/drivers/soc/rockchip/Kconfig
+++ b/drivers/soc/rockchip/Kconfig
@@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
 #
 # Rockchip Soc drivers
 #
+
+config ROCKCHIP_GRF
+	bool
+	default y
+	help
+	  The General Register Files are a central component providing
+	  special additional settings registers for a lot of soc-components.
+	  In a lot of cases there also need to be default settings initialized
+	  to make some of them conform to expectations of the kernel.
+
 config ROCKCHIP_PM_DOMAINS
         bool "Rockchip generic power domain"
         depends on PM
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
index 3d73d06..c851fa0 100644
--- a/drivers/soc/rockchip/Makefile
+++ b/drivers/soc/rockchip/Makefile
@@ -1,4 +1,5 @@
 #
 # Rockchip Soc drivers
 #
+obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
 obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
new file mode 100644
index 0000000..0c85476a
--- /dev/null
+++ b/drivers/soc/rockchip/grf.c
@@ -0,0 +1,134 @@
+/*
+ * Rockchip Generic Register Files setup
+ *
+ * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
+
+struct rockchip_grf_value {
+	const char *desc;
+	u32 reg;
+	u32 val;
+};
+
+struct rockchip_grf_info {
+	const struct rockchip_grf_value *values;
+	int num_values;
+};
+
+#define RK3036_GRF_SOC_CON0		0x140
+
+static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
+	/*
+	 * Disable auto jtag/sdmmc switching that causes issues with the
+	 * clock-framework and the mmc controllers making them unreliable.
+	 */
+	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
+};
+
+static const struct rockchip_grf_info rk3036_grf __initconst = {
+	.values = rk3036_defaults,
+	.num_values = ARRAY_SIZE(rk3036_defaults),
+};
+
+#define RK3288_GRF_SOC_CON0		0x244
+
+static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
+	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
+};
+
+static const struct rockchip_grf_info rk3288_grf __initconst = {
+	.values = rk3288_defaults,
+	.num_values = ARRAY_SIZE(rk3288_defaults),
+};
+
+#define RK3368_GRF_SOC_CON15		0x43c
+
+static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
+	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
+};
+
+static const struct rockchip_grf_info rk3368_grf __initconst = {
+	.values = rk3368_defaults,
+	.num_values = ARRAY_SIZE(rk3368_defaults),
+};
+
+#define RK3399_GRF_SOC_CON7		0xe21c
+
+static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
+	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
+};
+
+static const struct rockchip_grf_info rk3399_grf __initconst = {
+	.values = rk3399_defaults,
+	.num_values = ARRAY_SIZE(rk3399_defaults),
+};
+
+static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
+	{
+		.compatible = "rockchip,rk3036-grf",
+		.data = (void *)&rk3036_grf,
+	}, {
+		.compatible = "rockchip,rk3288-grf",
+		.data = (void *)&rk3288_grf,
+	}, {
+		.compatible = "rockchip,rk3368-grf",
+		.data = (void *)&rk3368_grf,
+	}, {
+		.compatible = "rockchip,rk3399-grf",
+		.data = (void *)&rk3399_grf,
+	},
+	{ /* sentinel */ },
+};
+
+static int __init rockchip_grf_init(void)
+{
+	const struct rockchip_grf_info *grf_info;
+	const struct of_device_id *match;
+	struct device_node *np;
+	struct regmap *grf;
+	int ret, i;
+
+	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match, &match);
+	if (!np)
+		return -ENODEV;
+	if (!match || !match->data) {
+		pr_err("%s: missing grf data\n", __func__);
+		return -EINVAL;
+	}
+
+	grf_info = match->data;
+
+	grf = syscon_node_to_regmap(np);
+	if (IS_ERR(grf)) {
+		pr_err("%s: could not get grf syscon\n", __func__);
+		return PTR_ERR(grf);
+	}
+
+	for (i = 0; i < grf_info->num_values; i++) {
+		const struct rockchip_grf_value *val = &grf_info->values[i];
+
+		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
+			val->desc, val->reg, val->val);
+		ret = regmap_write(grf, val->reg, val->val);
+		if (ret < 0)
+			pr_err("%s: write to %#6x failed with %d\n",
+			       __func__, val->reg, ret);
+	}
+
+	return 0;
+}
+postcore_initcall(rockchip_grf_init);
-- 
2.10.2

[PATCH v2 3/3] ARM: rockchip: drop rk3288 jtag/mmc switch handling

[Heiko Stuebner]

We moved that functionality to a more generic place where it can also
be used for other socs, so drop it from architecture code.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 arch/arm/mach-rockchip/rockchip.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
index e7fdf06..0008783 100644
--- a/arch/arm/mach-rockchip/rockchip.c
+++ b/arch/arm/mach-rockchip/rockchip.c
@@ -29,13 +29,11 @@
 #include "core.h"
 #include "pm.h"
 
-#define RK3288_GRF_SOC_CON0 0x244
 #define RK3288_TIMER6_7_PHYS 0xff810000
 
 static void __init rockchip_timer_init(void)
 {
 	if (of_machine_is_compatible("rockchip,rk3288")) {
-		struct regmap *grf;
 		void __iomem *reg_base;
 
 		/*
@@ -54,16 +52,6 @@ static void __init rockchip_timer_init(void)
 		} else {
 			pr_err("rockchip: could not map timer7 registers\n");
 		}
-
-		/*
-		 * Disable auto jtag/sdmmc switching that causes issues
-		 * with the mmc controllers making them unreliable
-		 */
-		grf = syscon_regmap_lookup_by_compatible("rockchip,rk3288-grf");
-		if (!IS_ERR(grf))
-			regmap_write(grf, RK3288_GRF_SOC_CON0, 0x10000000);
-		else
-			pr_err("rockchip: could not get grf syscon\n");
 	}
 
 	of_clk_init(NULL);
-- 
2.10.2

[PATCH v2 3/3] ARM: rockchip: drop rk3288 jtag/mmc switch handling

[Heiko Stuebner]

We moved that functionality to a more generic place where it can also
be used for other socs, so drop it from architecture code.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 arch/arm/mach-rockchip/rockchip.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
index e7fdf06..0008783 100644
--- a/arch/arm/mach-rockchip/rockchip.c
+++ b/arch/arm/mach-rockchip/rockchip.c
@@ -29,13 +29,11 @@
 #include "core.h"
 #include "pm.h"
 
-#define RK3288_GRF_SOC_CON0 0x244
 #define RK3288_TIMER6_7_PHYS 0xff810000
 
 static void __init rockchip_timer_init(void)
 {
 	if (of_machine_is_compatible("rockchip,rk3288")) {
-		struct regmap *grf;
 		void __iomem *reg_base;
 
 		/*
@@ -54,16 +52,6 @@ static void __init rockchip_timer_init(void)
 		} else {
 			pr_err("rockchip: could not map timer7 registers\n");
 		}
-
-		/*
-		 * Disable auto jtag/sdmmc switching that causes issues
-		 * with the mmc controllers making them unreliable
-		 */
-		grf = syscon_regmap_lookup_by_compatible("rockchip,rk3288-grf");
-		if (!IS_ERR(grf))
-			regmap_write(grf, RK3288_GRF_SOC_CON0, 0x10000000);
-		else
-			pr_err("rockchip: could not get grf syscon\n");
 	}
 
 	of_clk_init(NULL);
-- 
2.10.2

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Shawn Lin]

在 2016/11/16 6:38, Heiko Stuebner 写道:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.

I hate force_jtag personally... :)

>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.

yes, I did this inside the loader.... when running arm64

>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 drivers/soc/rockchip/grf.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 7140ff8..20da55d 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>  #
>  # Rockchip Soc drivers
>  #
> +
> +config ROCKCHIP_GRF
> +	bool
> +	default y
> +	help
> +	  The General Register Files are a central component providing
> +	  special additional settings registers for a lot of soc-components.
> +	  In a lot of cases there also need to be default settings initialized
> +	  to make some of them conform to expectations of the kernel.
> +
>  config ROCKCHIP_PM_DOMAINS
>          bool "Rockchip generic power domain"
>          depends on PM
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 3d73d06..c851fa0 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -1,4 +1,5 @@
>  #
>  # Rockchip Soc drivers
>  #
> +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> new file mode 100644
> index 0000000..0c85476a
> --- /dev/null
> +++ b/drivers/soc/rockchip/grf.c
> @@ -0,0 +1,134 @@
> +/*
> + * Rockchip Generic Register Files setup
> + *
> + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

The order :)

> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> +
> +struct rockchip_grf_value {
> +	const char *desc;
> +	u32 reg;
> +	u32 val;
> +};
> +
> +struct rockchip_grf_info {
> +	const struct rockchip_grf_value *values;
> +	int num_values;
> +};
> +
> +#define RK3036_GRF_SOC_CON0		0x140
> +
> +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> +	/*
> +	 * Disable auto jtag/sdmmc switching that causes issues with the
> +	 * clock-framework and the mmc controllers making them unreliable.
> +	 */
> +	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> +};
> +
> +static const struct rockchip_grf_info rk3036_grf __initconst = {
> +	.values = rk3036_defaults,
> +	.num_values = ARRAY_SIZE(rk3036_defaults),
> +};
> +
> +#define RK3288_GRF_SOC_CON0		0x244
> +
> +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> +	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3288_grf __initconst = {
> +	.values = rk3288_defaults,
> +	.num_values = ARRAY_SIZE(rk3288_defaults),
> +};
> +
> +#define RK3368_GRF_SOC_CON15		0x43c
> +
> +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> +	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> +};
> +
> +static const struct rockchip_grf_info rk3368_grf __initconst = {
> +	.values = rk3368_defaults,
> +	.num_values = ARRAY_SIZE(rk3368_defaults),
> +};
> +
> +#define RK3399_GRF_SOC_CON7		0xe21c
> +
> +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> +	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3399_grf __initconst = {
> +	.values = rk3399_defaults,
> +	.num_values = ARRAY_SIZE(rk3399_defaults),
> +};
> +
> +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> +	{
> +		.compatible = "rockchip,rk3036-grf",
> +		.data = (void *)&rk3036_grf,
> +	}, {
> +		.compatible = "rockchip,rk3288-grf",
> +		.data = (void *)&rk3288_grf,
> +	}, {
> +		.compatible = "rockchip,rk3368-grf",
> +		.data = (void *)&rk3368_grf,
> +	}, {
> +		.compatible = "rockchip,rk3399-grf",
> +		.data = (void *)&rk3399_grf,
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static int __init rockchip_grf_init(void)
> +{
> +	const struct rockchip_grf_info *grf_info;
> +	const struct of_device_id *match;
> +	struct device_node *np;
> +	struct regmap *grf;
> +	int ret, i;
> +
> +	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match, &match);
> +	if (!np)
> +		return -ENODEV;
> +	if (!match || !match->data) {
> +		pr_err("%s: missing grf data\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	grf_info = match->data;
> +
> +	grf = syscon_node_to_regmap(np);
> +	if (IS_ERR(grf)) {
> +		pr_err("%s: could not get grf syscon\n", __func__);
> +		return PTR_ERR(grf);
> +	}
> +
> +	for (i = 0; i < grf_info->num_values; i++) {
> +		const struct rockchip_grf_value *val = &grf_info->values[i];
> +
> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> +			val->desc, val->reg, val->val);
> +		ret = regmap_write(grf, val->reg, val->val);
> +		if (ret < 0)
> +			pr_err("%s: write to %#6x failed with %d\n",
> +			       __func__, val->reg, ret);

So, when failing to do one of the settings, should we still let it goes?
Sometimes the log of postcore_initcall is easy to be neglected when
people finally find problems later but the very earlier log was missing
due to whatever reason like buffer limitation, etc.


> +	}
> +
> +	return 0;
> +}
> +postcore_initcall(rockchip_grf_init);
>


-- 
Best Regards
Shawn Lin


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Shawn Lin]

? 2016/11/16 6:38, Heiko Stuebner ??:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.

I hate force_jtag personally... :)

>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.

yes, I did this inside the loader.... when running arm64

>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 drivers/soc/rockchip/grf.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 7140ff8..20da55d 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>  #
>  # Rockchip Soc drivers
>  #
> +
> +config ROCKCHIP_GRF
> +	bool
> +	default y
> +	help
> +	  The General Register Files are a central component providing
> +	  special additional settings registers for a lot of soc-components.
> +	  In a lot of cases there also need to be default settings initialized
> +	  to make some of them conform to expectations of the kernel.
> +
>  config ROCKCHIP_PM_DOMAINS
>          bool "Rockchip generic power domain"
>          depends on PM
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 3d73d06..c851fa0 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -1,4 +1,5 @@
>  #
>  # Rockchip Soc drivers
>  #
> +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> new file mode 100644
> index 0000000..0c85476a
> --- /dev/null
> +++ b/drivers/soc/rockchip/grf.c
> @@ -0,0 +1,134 @@
> +/*
> + * Rockchip Generic Register Files setup
> + *
> + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

The order :)

> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> +
> +struct rockchip_grf_value {
> +	const char *desc;
> +	u32 reg;
> +	u32 val;
> +};
> +
> +struct rockchip_grf_info {
> +	const struct rockchip_grf_value *values;
> +	int num_values;
> +};
> +
> +#define RK3036_GRF_SOC_CON0		0x140
> +
> +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> +	/*
> +	 * Disable auto jtag/sdmmc switching that causes issues with the
> +	 * clock-framework and the mmc controllers making them unreliable.
> +	 */
> +	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> +};
> +
> +static const struct rockchip_grf_info rk3036_grf __initconst = {
> +	.values = rk3036_defaults,
> +	.num_values = ARRAY_SIZE(rk3036_defaults),
> +};
> +
> +#define RK3288_GRF_SOC_CON0		0x244
> +
> +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> +	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3288_grf __initconst = {
> +	.values = rk3288_defaults,
> +	.num_values = ARRAY_SIZE(rk3288_defaults),
> +};
> +
> +#define RK3368_GRF_SOC_CON15		0x43c
> +
> +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> +	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> +};
> +
> +static const struct rockchip_grf_info rk3368_grf __initconst = {
> +	.values = rk3368_defaults,
> +	.num_values = ARRAY_SIZE(rk3368_defaults),
> +};
> +
> +#define RK3399_GRF_SOC_CON7		0xe21c
> +
> +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> +	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3399_grf __initconst = {
> +	.values = rk3399_defaults,
> +	.num_values = ARRAY_SIZE(rk3399_defaults),
> +};
> +
> +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> +	{
> +		.compatible = "rockchip,rk3036-grf",
> +		.data = (void *)&rk3036_grf,
> +	}, {
> +		.compatible = "rockchip,rk3288-grf",
> +		.data = (void *)&rk3288_grf,
> +	}, {
> +		.compatible = "rockchip,rk3368-grf",
> +		.data = (void *)&rk3368_grf,
> +	}, {
> +		.compatible = "rockchip,rk3399-grf",
> +		.data = (void *)&rk3399_grf,
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static int __init rockchip_grf_init(void)
> +{
> +	const struct rockchip_grf_info *grf_info;
> +	const struct of_device_id *match;
> +	struct device_node *np;
> +	struct regmap *grf;
> +	int ret, i;
> +
> +	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match, &match);
> +	if (!np)
> +		return -ENODEV;
> +	if (!match || !match->data) {
> +		pr_err("%s: missing grf data\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	grf_info = match->data;
> +
> +	grf = syscon_node_to_regmap(np);
> +	if (IS_ERR(grf)) {
> +		pr_err("%s: could not get grf syscon\n", __func__);
> +		return PTR_ERR(grf);
> +	}
> +
> +	for (i = 0; i < grf_info->num_values; i++) {
> +		const struct rockchip_grf_value *val = &grf_info->values[i];
> +
> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> +			val->desc, val->reg, val->val);
> +		ret = regmap_write(grf, val->reg, val->val);
> +		if (ret < 0)
> +			pr_err("%s: write to %#6x failed with %d\n",
> +			       __func__, val->reg, ret);

So, when failing to do one of the settings, should we still let it goes?
Sometimes the log of postcore_initcall is easy to be neglected when
people finally find problems later but the very earlier log was missing
due to whatever reason like buffer limitation, etc.


> +	}
> +
> +	return 0;
> +}
> +postcore_initcall(rockchip_grf_init);
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Shawn,

Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
> 在 2016/11/16 6:38, Heiko Stuebner 写道:
> > The General Register Files are an area of registers containing a lot
> > of single-bit settings for numerous components as well full components
> > like usbphy control. Therefore all used components are accessed
> > via the syscon provided by the grf nodes or from the sub-devices
> > created through the simple-mfd created from the grf node.
> > 
> > Some settings are not used by anything but will need to be set up
> > according to expectations on the kernel side.
> > 
> > Best example is the force_jtag setting, which defaults to on and
> > results in the soc switching the pin-outputs between jtag and sdmmc
> > automatically depending on the card-detect status. This conflicts
> > heavily with how the dw_mmc driver expects to do its work and also
> > with the clock-controller, which has most likely deactivated the
> > jtag clock due to it being unused.
> 
> I hate force_jtag personally... :)

yep, I still remember when we had strange mmc errors which simply came from 
the soc switching away from the mmc function before the mmc driver was
finished, so I also find that functionality unhelpful ;-) .

> > So far the handling of this setting was living in the mach-rockchip
> > code for the arm32-based rk3288 but that of course doesn't work
> > for arm64 socs and would also look ugly for further arm32 socs.
> 
> yes, I did this inside the loader.... when running arm64
> 
> > Also always disabling this setting is quite specific to linux and
> > its subsystems, other operating systems might prefer other settings,
> > so that the bootloader cannot really set a sane default for all.
> > 
> > So introduce a top-level driver for the grf that handles these
> > settings that need to be a certain way but nobody cares about.
> > 
> > Other needed settings might surface in the future and can then
> > be added here, but only as a last option. Ideally general GRF
> > settings should be handled in the driver needing them.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/soc/rockchip/Kconfig  |  10 ++++
> >  drivers/soc/rockchip/Makefile |   1 +
> >  drivers/soc/rockchip/grf.c    | 134
> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
> >  insertions(+)
> >  create mode 100644 drivers/soc/rockchip/grf.c
> > 
> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> > index 7140ff8..20da55d 100644
> > --- a/drivers/soc/rockchip/Kconfig
> > +++ b/drivers/soc/rockchip/Kconfig
> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +
> > +config ROCKCHIP_GRF
> > +	bool
> > +	default y
> > +	help
> > +	  The General Register Files are a central component providing
> > +	  special additional settings registers for a lot of soc-components.
> > +	  In a lot of cases there also need to be default settings initialized
> > +	  to make some of them conform to expectations of the kernel.
> > +
> > 
> >  config ROCKCHIP_PM_DOMAINS
> >  
> >          bool "Rockchip generic power domain"
> >          depends on PM
> > 
> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> > index 3d73d06..c851fa0 100644
> > --- a/drivers/soc/rockchip/Makefile
> > +++ b/drivers/soc/rockchip/Makefile
> > @@ -1,4 +1,5 @@
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
> > 
> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> > 
> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> > new file mode 100644
> > index 0000000..0c85476a
> > --- /dev/null
> > +++ b/drivers/soc/rockchip/grf.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Rockchip Generic Register Files setup
> > + *
> > + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> The order :)

I just noticed that there is a second inclusion of regmap.h here, which can go 
away.


> 
> > +
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +		((val) << (shift) | (mask) << ((shift) + 16))
> > +
> > +struct rockchip_grf_value {
> > +	const char *desc;
> > +	u32 reg;
> > +	u32 val;
> > +};
> > +
> > +struct rockchip_grf_info {
> > +	const struct rockchip_grf_value *values;
> > +	int num_values;
> > +};
> > +
> > +#define RK3036_GRF_SOC_CON0		0x140
> > +
> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> > +	/*
> > +	 * Disable auto jtag/sdmmc switching that causes issues with the
> > +	 * clock-framework and the mmc controllers making them unreliable.
> > +	 */
> > +	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
> > +	.values = rk3036_defaults,
> > +	.num_values = ARRAY_SIZE(rk3036_defaults),
> > +};
> > +
> > +#define RK3288_GRF_SOC_CON0		0x244
> > +
> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> > +	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
> > +	.values = rk3288_defaults,
> > +	.num_values = ARRAY_SIZE(rk3288_defaults),
> > +};
> > +
> > +#define RK3368_GRF_SOC_CON15		0x43c
> > +
> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> > +	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
> > +	.values = rk3368_defaults,
> > +	.num_values = ARRAY_SIZE(rk3368_defaults),
> > +};
> > +
> > +#define RK3399_GRF_SOC_CON7		0xe21c
> > +
> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> > +	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
> > +	.values = rk3399_defaults,
> > +	.num_values = ARRAY_SIZE(rk3399_defaults),
> > +};
> > +
> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> > +	{
> > +		.compatible = "rockchip,rk3036-grf",
> > +		.data = (void *)&rk3036_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3288-grf",
> > +		.data = (void *)&rk3288_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3368-grf",
> > +		.data = (void *)&rk3368_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3399-grf",
> > +		.data = (void *)&rk3399_grf,
> > +	},
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static int __init rockchip_grf_init(void)
> > +{
> > +	const struct rockchip_grf_info *grf_info;
> > +	const struct of_device_id *match;
> > +	struct device_node *np;
> > +	struct regmap *grf;
> > +	int ret, i;
> > +
> > +	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match,
> > &match); +	if (!np)
> > +		return -ENODEV;
> > +	if (!match || !match->data) {
> > +		pr_err("%s: missing grf data\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	grf_info = match->data;
> > +
> > +	grf = syscon_node_to_regmap(np);
> > +	if (IS_ERR(grf)) {
> > +		pr_err("%s: could not get grf syscon\n", __func__);
> > +		return PTR_ERR(grf);
> > +	}
> > +
> > +	for (i = 0; i < grf_info->num_values; i++) {
> > +		const struct rockchip_grf_value *val = &grf_info->values[i];
> > +
> > +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> > +			val->desc, val->reg, val->val);
> > +		ret = regmap_write(grf, val->reg, val->val);
> > +		if (ret < 0)
> > +			pr_err("%s: write to %#6x failed with %d\n",
> > +			       __func__, val->reg, ret);
> 
> So, when failing to do one of the settings, should we still let it goes?
> Sometimes the log of postcore_initcall is easy to be neglected when
> people finally find problems later but the very earlier log was missing
> due to whatever reason like buffer limitation, etc.

I expect errors here to be very rare. I.e. Doug thought that regmap might 
return errors if we write outside the mapped region, which would mean someone 
really messed up in this core component or a core-element of the dts.
But in general the GRF is always a mmio-regmap, so there won't be any i2c/spi/
whatever errors possible.

Also settings are pretty individual, so if setting 1 fails, setting 2 can 
still succeed and the boot can continue somewhat farther.

The overall target is supposed to always boot as far as possible, so that 
people might recover or get more information and not fail (and die) early.


Heiko

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Shawn,

Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
> ? 2016/11/16 6:38, Heiko Stuebner ??:
> > The General Register Files are an area of registers containing a lot
> > of single-bit settings for numerous components as well full components
> > like usbphy control. Therefore all used components are accessed
> > via the syscon provided by the grf nodes or from the sub-devices
> > created through the simple-mfd created from the grf node.
> > 
> > Some settings are not used by anything but will need to be set up
> > according to expectations on the kernel side.
> > 
> > Best example is the force_jtag setting, which defaults to on and
> > results in the soc switching the pin-outputs between jtag and sdmmc
> > automatically depending on the card-detect status. This conflicts
> > heavily with how the dw_mmc driver expects to do its work and also
> > with the clock-controller, which has most likely deactivated the
> > jtag clock due to it being unused.
> 
> I hate force_jtag personally... :)

yep, I still remember when we had strange mmc errors which simply came from 
the soc switching away from the mmc function before the mmc driver was
finished, so I also find that functionality unhelpful ;-) .

> > So far the handling of this setting was living in the mach-rockchip
> > code for the arm32-based rk3288 but that of course doesn't work
> > for arm64 socs and would also look ugly for further arm32 socs.
> 
> yes, I did this inside the loader.... when running arm64
> 
> > Also always disabling this setting is quite specific to linux and
> > its subsystems, other operating systems might prefer other settings,
> > so that the bootloader cannot really set a sane default for all.
> > 
> > So introduce a top-level driver for the grf that handles these
> > settings that need to be a certain way but nobody cares about.
> > 
> > Other needed settings might surface in the future and can then
> > be added here, but only as a last option. Ideally general GRF
> > settings should be handled in the driver needing them.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/soc/rockchip/Kconfig  |  10 ++++
> >  drivers/soc/rockchip/Makefile |   1 +
> >  drivers/soc/rockchip/grf.c    | 134
> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
> >  insertions(+)
> >  create mode 100644 drivers/soc/rockchip/grf.c
> > 
> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> > index 7140ff8..20da55d 100644
> > --- a/drivers/soc/rockchip/Kconfig
> > +++ b/drivers/soc/rockchip/Kconfig
> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +
> > +config ROCKCHIP_GRF
> > +	bool
> > +	default y
> > +	help
> > +	  The General Register Files are a central component providing
> > +	  special additional settings registers for a lot of soc-components.
> > +	  In a lot of cases there also need to be default settings initialized
> > +	  to make some of them conform to expectations of the kernel.
> > +
> > 
> >  config ROCKCHIP_PM_DOMAINS
> >  
> >          bool "Rockchip generic power domain"
> >          depends on PM
> > 
> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> > index 3d73d06..c851fa0 100644
> > --- a/drivers/soc/rockchip/Makefile
> > +++ b/drivers/soc/rockchip/Makefile
> > @@ -1,4 +1,5 @@
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
> > 
> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> > 
> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> > new file mode 100644
> > index 0000000..0c85476a
> > --- /dev/null
> > +++ b/drivers/soc/rockchip/grf.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Rockchip Generic Register Files setup
> > + *
> > + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> The order :)

I just noticed that there is a second inclusion of regmap.h here, which can go 
away.


> 
> > +
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +		((val) << (shift) | (mask) << ((shift) + 16))
> > +
> > +struct rockchip_grf_value {
> > +	const char *desc;
> > +	u32 reg;
> > +	u32 val;
> > +};
> > +
> > +struct rockchip_grf_info {
> > +	const struct rockchip_grf_value *values;
> > +	int num_values;
> > +};
> > +
> > +#define RK3036_GRF_SOC_CON0		0x140
> > +
> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> > +	/*
> > +	 * Disable auto jtag/sdmmc switching that causes issues with the
> > +	 * clock-framework and the mmc controllers making them unreliable.
> > +	 */
> > +	{ "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
> > +	.values = rk3036_defaults,
> > +	.num_values = ARRAY_SIZE(rk3036_defaults),
> > +};
> > +
> > +#define RK3288_GRF_SOC_CON0		0x244
> > +
> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> > +	{ "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
> > +	.values = rk3288_defaults,
> > +	.num_values = ARRAY_SIZE(rk3288_defaults),
> > +};
> > +
> > +#define RK3368_GRF_SOC_CON15		0x43c
> > +
> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> > +	{ "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
> > +	.values = rk3368_defaults,
> > +	.num_values = ARRAY_SIZE(rk3368_defaults),
> > +};
> > +
> > +#define RK3399_GRF_SOC_CON7		0xe21c
> > +
> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> > +	{ "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
> > +	.values = rk3399_defaults,
> > +	.num_values = ARRAY_SIZE(rk3399_defaults),
> > +};
> > +
> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> > +	{
> > +		.compatible = "rockchip,rk3036-grf",
> > +		.data = (void *)&rk3036_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3288-grf",
> > +		.data = (void *)&rk3288_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3368-grf",
> > +		.data = (void *)&rk3368_grf,
> > +	}, {
> > +		.compatible = "rockchip,rk3399-grf",
> > +		.data = (void *)&rk3399_grf,
> > +	},
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static int __init rockchip_grf_init(void)
> > +{
> > +	const struct rockchip_grf_info *grf_info;
> > +	const struct of_device_id *match;
> > +	struct device_node *np;
> > +	struct regmap *grf;
> > +	int ret, i;
> > +
> > +	np = of_find_matching_node_and_match(NULL, rockchip_grf_dt_match,
> > &match); +	if (!np)
> > +		return -ENODEV;
> > +	if (!match || !match->data) {
> > +		pr_err("%s: missing grf data\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	grf_info = match->data;
> > +
> > +	grf = syscon_node_to_regmap(np);
> > +	if (IS_ERR(grf)) {
> > +		pr_err("%s: could not get grf syscon\n", __func__);
> > +		return PTR_ERR(grf);
> > +	}
> > +
> > +	for (i = 0; i < grf_info->num_values; i++) {
> > +		const struct rockchip_grf_value *val = &grf_info->values[i];
> > +
> > +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> > +			val->desc, val->reg, val->val);
> > +		ret = regmap_write(grf, val->reg, val->val);
> > +		if (ret < 0)
> > +			pr_err("%s: write to %#6x failed with %d\n",
> > +			       __func__, val->reg, ret);
> 
> So, when failing to do one of the settings, should we still let it goes?
> Sometimes the log of postcore_initcall is easy to be neglected when
> people finally find problems later but the very earlier log was missing
> due to whatever reason like buffer limitation, etc.

I expect errors here to be very rare. I.e. Doug thought that regmap might 
return errors if we write outside the mapped region, which would mean someone 
really messed up in this core component or a core-element of the dts.
But in general the GRF is always a mmio-regmap, so there won't be any i2c/spi/
whatever errors possible.

Also settings are pretty individual, so if setting 1 fails, setting 2 can 
still succeed and the boot can continue somewhat farther.

The overall target is supposed to always boot as far as possible, so that 
people might recover or get more information and not fail (and die) early.


Heiko

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Doug Anderson]

Heiko,

On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.
>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.
>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)

Other than the header file stuff pointed out by Shawn, this looks good
to me now.

Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Doug Anderson]

Heiko,

On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.
>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.
>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)

Other than the header file stuff pointed out by Shawn, this looks good
to me now.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Shawn Lin]

Hi Heiko,

在 2016/11/16 17:58, Heiko Stuebner 写道:
> Hi Shawn,
>
> Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
>> 在 2016/11/16 6:38, Heiko Stuebner 写道:
>>> The General Register Files are an area of registers containing a lot
>>> of single-bit settings for numerous components as well full components
>>> like usbphy control. Therefore all used components are accessed
>>> via the syscon provided by the grf nodes or from the sub-devices
>>> created through the simple-mfd created from the grf node.
>>>

----8<----------------
[...]

>>> +	for (i = 0; i < grf_info->num_values; i++) {
>>> +		const struct rockchip_grf_value *val = &grf_info->values[i];
>>> +
>>> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
>>> +			val->desc, val->reg, val->val);
>>> +		ret = regmap_write(grf, val->reg, val->val);
>>> +		if (ret < 0)
>>> +			pr_err("%s: write to %#6x failed with %d\n",
>>> +			       __func__, val->reg, ret);
>>
>> So, when failing to do one of the settings, should we still let it goes?
>> Sometimes the log of postcore_initcall is easy to be neglected when
>> people finally find problems later but the very earlier log was missing
>> due to whatever reason like buffer limitation, etc.
>
> I expect errors here to be very rare. I.e. Doug thought that regmap might
> return errors if we write outside the mapped region, which would mean someone
> really messed up in this core component or a core-element of the dts.
> But in general the GRF is always a mmio-regmap, so there won't be any i2c/spi/
> whatever errors possible.

I was just thinking about the scalability that in the future some of the
GRF settings may depend on genpd or clock even if they are only a
mmio-regmap but they don't belong to the general pd/clock for GRF, for
instance, some of the PHYs' or controller's cap(like emmc) settings.

But, IIRC, you suggested this driver shouldn't be a catchall, and we
now ask the drivers of controllers and phys to take over this, so I
guess we won't put those settings here ever. :)

Thanks for explaining this.

>
> Also settings are pretty individual, so if setting 1 fails, setting 2 can
> still succeed and the boot can continue somewhat farther.
>

sure

> The overall target is supposed to always boot as far as possible, so that
> people might recover or get more information and not fail (and die) early.
>

ok, got it.

>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Shawn Lin]

Hi Heiko,

? 2016/11/16 17:58, Heiko Stuebner ??:
> Hi Shawn,
>
> Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
>> ? 2016/11/16 6:38, Heiko Stuebner ??:
>>> The General Register Files are an area of registers containing a lot
>>> of single-bit settings for numerous components as well full components
>>> like usbphy control. Therefore all used components are accessed
>>> via the syscon provided by the grf nodes or from the sub-devices
>>> created through the simple-mfd created from the grf node.
>>>

----8<----------------
[...]

>>> +	for (i = 0; i < grf_info->num_values; i++) {
>>> +		const struct rockchip_grf_value *val = &grf_info->values[i];
>>> +
>>> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
>>> +			val->desc, val->reg, val->val);
>>> +		ret = regmap_write(grf, val->reg, val->val);
>>> +		if (ret < 0)
>>> +			pr_err("%s: write to %#6x failed with %d\n",
>>> +			       __func__, val->reg, ret);
>>
>> So, when failing to do one of the settings, should we still let it goes?
>> Sometimes the log of postcore_initcall is easy to be neglected when
>> people finally find problems later but the very earlier log was missing
>> due to whatever reason like buffer limitation, etc.
>
> I expect errors here to be very rare. I.e. Doug thought that regmap might
> return errors if we write outside the mapped region, which would mean someone
> really messed up in this core component or a core-element of the dts.
> But in general the GRF is always a mmio-regmap, so there won't be any i2c/spi/
> whatever errors possible.

I was just thinking about the scalability that in the future some of the
GRF settings may depend on genpd or clock even if they are only a
mmio-regmap but they don't belong to the general pd/clock for GRF, for
instance, some of the PHYs' or controller's cap(like emmc) settings.

But, IIRC, you suggested this driver shouldn't be a catchall, and we
now ask the drivers of controllers and phys to take over this, so I
guess we won't put those settings here ever. :)

Thanks for explaining this.

>
> Also settings are pretty individual, so if setting 1 fails, setting 2 can
> still succeed and the boot can continue somewhat farther.
>

sure

> The overall target is supposed to always boot as far as possible, so that
> people might recover or get more information and not fail (and die) early.
>

ok, got it.

>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Am Donnerstag, 17. November 2016, 09:38:11 CET schrieb Shawn Lin:
> Hi Heiko,
> 
> 在 2016/11/16 17:58, Heiko Stuebner 写道:
> > Hi Shawn,
> > 
> > Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
> >> 在 2016/11/16 6:38, Heiko Stuebner 写道:
> >>> The General Register Files are an area of registers containing a lot
> >>> of single-bit settings for numerous components as well full components
> >>> like usbphy control. Therefore all used components are accessed
> >>> via the syscon provided by the grf nodes or from the sub-devices
> >>> created through the simple-mfd created from the grf node.
> 
> ----8<----------------
> [...]
> 
> >>> +	for (i = 0; i < grf_info->num_values; i++) {
> >>> +		const struct rockchip_grf_value *val = &grf_info->values[i];
> >>> +
> >>> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> >>> +			val->desc, val->reg, val->val);
> >>> +		ret = regmap_write(grf, val->reg, val->val);
> >>> +		if (ret < 0)
> >>> +			pr_err("%s: write to %#6x failed with %d\n",
> >>> +			       __func__, val->reg, ret);
> >> 
> >> So, when failing to do one of the settings, should we still let it goes?
> >> Sometimes the log of postcore_initcall is easy to be neglected when
> >> people finally find problems later but the very earlier log was missing
> >> due to whatever reason like buffer limitation, etc.
> > 
> > I expect errors here to be very rare. I.e. Doug thought that regmap might
> > return errors if we write outside the mapped region, which would mean
> > someone really messed up in this core component or a core-element of the
> > dts. But in general the GRF is always a mmio-regmap, so there won't be
> > any i2c/spi/ whatever errors possible.
> 
> I was just thinking about the scalability that in the future some of the
> GRF settings may depend on genpd or clock even if they are only a
> mmio-regmap but they don't belong to the general pd/clock for GRF, for
> instance, some of the PHYs' or controller's cap(like emmc) settings.
> 
> But, IIRC, you suggested this driver shouldn't be a catchall, and we
> now ask the drivers of controllers and phys to take over this, so I
> guess we won't put those settings here ever. :)
> 
> Thanks for explaining this.

correct, it should never become a catchall as things like those phy-subdevices 
that need runtime handling should definitly be handled in their respective 
drivers.

So far we're also always starting with all clocks and power-domains being on, 
so settings should always succeed as we're only active during early boot here.
We'll simply have to reevaluate if some new soc begins to boot with some 
needed clocks/power-domains being off.


Heiko

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Am Donnerstag, 17. November 2016, 09:38:11 CET schrieb Shawn Lin:
> Hi Heiko,
> 
> ? 2016/11/16 17:58, Heiko Stuebner ??:
> > Hi Shawn,
> > 
> > Am Mittwoch, 16. November 2016, 17:33:21 CET schrieb Shawn Lin:
> >> ? 2016/11/16 6:38, Heiko Stuebner ??:
> >>> The General Register Files are an area of registers containing a lot
> >>> of single-bit settings for numerous components as well full components
> >>> like usbphy control. Therefore all used components are accessed
> >>> via the syscon provided by the grf nodes or from the sub-devices
> >>> created through the simple-mfd created from the grf node.
> 
> ----8<----------------
> [...]
> 
> >>> +	for (i = 0; i < grf_info->num_values; i++) {
> >>> +		const struct rockchip_grf_value *val = &grf_info->values[i];
> >>> +
> >>> +		pr_debug("%s: adjusting %s in %#6x to %#10x\n", __func__,
> >>> +			val->desc, val->reg, val->val);
> >>> +		ret = regmap_write(grf, val->reg, val->val);
> >>> +		if (ret < 0)
> >>> +			pr_err("%s: write to %#6x failed with %d\n",
> >>> +			       __func__, val->reg, ret);
> >> 
> >> So, when failing to do one of the settings, should we still let it goes?
> >> Sometimes the log of postcore_initcall is easy to be neglected when
> >> people finally find problems later but the very earlier log was missing
> >> due to whatever reason like buffer limitation, etc.
> > 
> > I expect errors here to be very rare. I.e. Doug thought that regmap might
> > return errors if we write outside the mapped region, which would mean
> > someone really messed up in this core component or a core-element of the
> > dts. But in general the GRF is always a mmio-regmap, so there won't be
> > any i2c/spi/ whatever errors possible.
> 
> I was just thinking about the scalability that in the future some of the
> GRF settings may depend on genpd or clock even if they are only a
> mmio-regmap but they don't belong to the general pd/clock for GRF, for
> instance, some of the PHYs' or controller's cap(like emmc) settings.
> 
> But, IIRC, you suggested this driver shouldn't be a catchall, and we
> now ask the drivers of controllers and phys to take over this, so I
> guess we won't put those settings here ever. :)
> 
> Thanks for explaining this.

correct, it should never become a catchall as things like those phy-subdevices 
that need runtime handling should definitly be handled in their respective 
drivers.

So far we're also always starting with all clocks and power-domains being on, 
so settings should always succeed as we're only active during early boot here.
We'll simply have to reevaluate if some new soc begins to boot with some 
needed clocks/power-domains being off.


Heiko

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Olof Johansson]

Heiko,



On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.
>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.
>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 drivers/soc/rockchip/grf.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 7140ff8..20da55d 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>  #
>  # Rockchip Soc drivers
>  #
> +
> +config ROCKCHIP_GRF
> +       bool
> +       default y
> +       help
> +         The General Register Files are a central component providing
> +         special additional settings registers for a lot of soc-components.
> +         In a lot of cases there also need to be default settings initialized
> +         to make some of them conform to expectations of the kernel.
> +
>  config ROCKCHIP_PM_DOMAINS
>          bool "Rockchip generic power domain"
>          depends on PM
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 3d73d06..c851fa0 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -1,4 +1,5 @@
>  #
>  # Rockchip Soc drivers
>  #
> +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> new file mode 100644
> index 0000000..0c85476a
> --- /dev/null
> +++ b/drivers/soc/rockchip/grf.c
> @@ -0,0 +1,134 @@
> +/*
> + * Rockchip Generic Register Files setup
> + *
> + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +               ((val) << (shift) | (mask) << ((shift) + 16))
> +
> +struct rockchip_grf_value {
> +       const char *desc;
> +       u32 reg;
> +       u32 val;
> +};
> +
> +struct rockchip_grf_info {
> +       const struct rockchip_grf_value *values;
> +       int num_values;
> +};
> +
> +#define RK3036_GRF_SOC_CON0            0x140
> +
> +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> +       /*
> +        * Disable auto jtag/sdmmc switching that causes issues with the
> +        * clock-framework and the mmc controllers making them unreliable.
> +        */
> +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> +};
> +
> +static const struct rockchip_grf_info rk3036_grf __initconst = {
> +       .values = rk3036_defaults,
> +       .num_values = ARRAY_SIZE(rk3036_defaults),
> +};
> +
> +#define RK3288_GRF_SOC_CON0            0x244
> +
> +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3288_grf __initconst = {
> +       .values = rk3288_defaults,
> +       .num_values = ARRAY_SIZE(rk3288_defaults),
> +};
> +
> +#define RK3368_GRF_SOC_CON15           0x43c
> +
> +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> +};
> +
> +static const struct rockchip_grf_info rk3368_grf __initconst = {
> +       .values = rk3368_defaults,
> +       .num_values = ARRAY_SIZE(rk3368_defaults),
> +};
> +
> +#define RK3399_GRF_SOC_CON7            0xe21c
> +
> +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3399_grf __initconst = {
> +       .values = rk3399_defaults,
> +       .num_values = ARRAY_SIZE(rk3399_defaults),
> +};
> +
> +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> +       {
> +               .compatible = "rockchip,rk3036-grf",
> +               .data = (void *)&rk3036_grf,
> +       }, {
> +               .compatible = "rockchip,rk3288-grf",
> +               .data = (void *)&rk3288_grf,
> +       }, {
> +               .compatible = "rockchip,rk3368-grf",
> +               .data = (void *)&rk3368_grf,
> +       }, {
> +               .compatible = "rockchip,rk3399-grf",
> +               .data = (void *)&rk3399_grf,
> +       },
> +       { /* sentinel */ },
> +};

I often come in after there's already been discussion on a topic, and
don't always find all of it, so let me know if this has already been
considered and not chosen for some reason:


I get a little worried when you see these per-SoC tables build up in
the kernel. It means there'll need to be additions here for a bunch of
different SoCs.

Would it be possible to describe this directly in the DT instead? If
nothing else, something like

function-regs = < array of regs >
function-values = < array of values >
function-names = < array of names >

Not ideal either, especially if abused into a random poke table, but
it won't require per-SoC changes to the kernel. If we keep the binding
under close eyes we can hopefully avoid abuse.

Given that Rockchip is still working on new SoCs, this list will just
grow and doing it in-kernel will stop scaling at some point.


-Olof

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Olof Johansson]

Heiko,



On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> The General Register Files are an area of registers containing a lot
> of single-bit settings for numerous components as well full components
> like usbphy control. Therefore all used components are accessed
> via the syscon provided by the grf nodes or from the sub-devices
> created through the simple-mfd created from the grf node.
>
> Some settings are not used by anything but will need to be set up
> according to expectations on the kernel side.
>
> Best example is the force_jtag setting, which defaults to on and
> results in the soc switching the pin-outputs between jtag and sdmmc
> automatically depending on the card-detect status. This conflicts
> heavily with how the dw_mmc driver expects to do its work and also
> with the clock-controller, which has most likely deactivated the
> jtag clock due to it being unused.
>
> So far the handling of this setting was living in the mach-rockchip
> code for the arm32-based rk3288 but that of course doesn't work
> for arm64 socs and would also look ugly for further arm32 socs.
>
> Also always disabling this setting is quite specific to linux and
> its subsystems, other operating systems might prefer other settings,
> so that the bootloader cannot really set a sane default for all.
>
> So introduce a top-level driver for the grf that handles these
> settings that need to be a certain way but nobody cares about.
>
> Other needed settings might surface in the future and can then
> be added here, but only as a last option. Ideally general GRF
> settings should be handled in the driver needing them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/soc/rockchip/Kconfig  |  10 ++++
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/grf.c    | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 drivers/soc/rockchip/grf.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 7140ff8..20da55d 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>  #
>  # Rockchip Soc drivers
>  #
> +
> +config ROCKCHIP_GRF
> +       bool
> +       default y
> +       help
> +         The General Register Files are a central component providing
> +         special additional settings registers for a lot of soc-components.
> +         In a lot of cases there also need to be default settings initialized
> +         to make some of them conform to expectations of the kernel.
> +
>  config ROCKCHIP_PM_DOMAINS
>          bool "Rockchip generic power domain"
>          depends on PM
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 3d73d06..c851fa0 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -1,4 +1,5 @@
>  #
>  # Rockchip Soc drivers
>  #
> +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> new file mode 100644
> index 0000000..0c85476a
> --- /dev/null
> +++ b/drivers/soc/rockchip/grf.c
> @@ -0,0 +1,134 @@
> +/*
> + * Rockchip Generic Register Files setup
> + *
> + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +               ((val) << (shift) | (mask) << ((shift) + 16))
> +
> +struct rockchip_grf_value {
> +       const char *desc;
> +       u32 reg;
> +       u32 val;
> +};
> +
> +struct rockchip_grf_info {
> +       const struct rockchip_grf_value *values;
> +       int num_values;
> +};
> +
> +#define RK3036_GRF_SOC_CON0            0x140
> +
> +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> +       /*
> +        * Disable auto jtag/sdmmc switching that causes issues with the
> +        * clock-framework and the mmc controllers making them unreliable.
> +        */
> +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11) },
> +};
> +
> +static const struct rockchip_grf_info rk3036_grf __initconst = {
> +       .values = rk3036_defaults,
> +       .num_values = ARRAY_SIZE(rk3036_defaults),
> +};
> +
> +#define RK3288_GRF_SOC_CON0            0x244
> +
> +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3288_grf __initconst = {
> +       .values = rk3288_defaults,
> +       .num_values = ARRAY_SIZE(rk3288_defaults),
> +};
> +
> +#define RK3368_GRF_SOC_CON15           0x43c
> +
> +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13) },
> +};
> +
> +static const struct rockchip_grf_info rk3368_grf __initconst = {
> +       .values = rk3368_defaults,
> +       .num_values = ARRAY_SIZE(rk3368_defaults),
> +};
> +
> +#define RK3399_GRF_SOC_CON7            0xe21c
> +
> +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12) },
> +};
> +
> +static const struct rockchip_grf_info rk3399_grf __initconst = {
> +       .values = rk3399_defaults,
> +       .num_values = ARRAY_SIZE(rk3399_defaults),
> +};
> +
> +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> +       {
> +               .compatible = "rockchip,rk3036-grf",
> +               .data = (void *)&rk3036_grf,
> +       }, {
> +               .compatible = "rockchip,rk3288-grf",
> +               .data = (void *)&rk3288_grf,
> +       }, {
> +               .compatible = "rockchip,rk3368-grf",
> +               .data = (void *)&rk3368_grf,
> +       }, {
> +               .compatible = "rockchip,rk3399-grf",
> +               .data = (void *)&rk3399_grf,
> +       },
> +       { /* sentinel */ },
> +};

I often come in after there's already been discussion on a topic, and
don't always find all of it, so let me know if this has already been
considered and not chosen for some reason:


I get a little worried when you see these per-SoC tables build up in
the kernel. It means there'll need to be additions here for a bunch of
different SoCs.

Would it be possible to describe this directly in the DT instead? If
nothing else, something like

function-regs = < array of regs >
function-values = < array of values >
function-names = < array of names >

Not ideal either, especially if abused into a random poke table, but
it won't require per-SoC changes to the kernel. If we keep the binding
under close eyes we can hopefully avoid abuse.

Given that Rockchip is still working on new SoCs, this list will just
grow and doing it in-kernel will stop scaling at some point.


-Olof

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Olof,

Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > The General Register Files are an area of registers containing a lot
> > of single-bit settings for numerous components as well full components
> > like usbphy control. Therefore all used components are accessed
> > via the syscon provided by the grf nodes or from the sub-devices
> > created through the simple-mfd created from the grf node.
> > 
> > Some settings are not used by anything but will need to be set up
> > according to expectations on the kernel side.
> > 
> > Best example is the force_jtag setting, which defaults to on and
> > results in the soc switching the pin-outputs between jtag and sdmmc
> > automatically depending on the card-detect status. This conflicts
> > heavily with how the dw_mmc driver expects to do its work and also
> > with the clock-controller, which has most likely deactivated the
> > jtag clock due to it being unused.
> > 
> > So far the handling of this setting was living in the mach-rockchip
> > code for the arm32-based rk3288 but that of course doesn't work
> > for arm64 socs and would also look ugly for further arm32 socs.
> > 
> > Also always disabling this setting is quite specific to linux and
> > its subsystems, other operating systems might prefer other settings,
> > so that the bootloader cannot really set a sane default for all.
> > 
> > So introduce a top-level driver for the grf that handles these
> > settings that need to be a certain way but nobody cares about.
> > 
> > Other needed settings might surface in the future and can then
> > be added here, but only as a last option. Ideally general GRF
> > settings should be handled in the driver needing them.
> > 
> > Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> > ---
> > 
> >  drivers/soc/rockchip/Kconfig  |  10 ++++
> >  drivers/soc/rockchip/Makefile |   1 +
> >  drivers/soc/rockchip/grf.c    | 134
> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
> >  insertions(+)
> >  create mode 100644 drivers/soc/rockchip/grf.c
> > 
> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> > index 7140ff8..20da55d 100644
> > --- a/drivers/soc/rockchip/Kconfig
> > +++ b/drivers/soc/rockchip/Kconfig
> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +
> > +config ROCKCHIP_GRF
> > +       bool
> > +       default y
> > +       help
> > +         The General Register Files are a central component providing
> > +         special additional settings registers for a lot of
> > soc-components. +         In a lot of cases there also need to be default
> > settings initialized +         to make some of them conform to
> > expectations of the kernel. +
> > 
> >  config ROCKCHIP_PM_DOMAINS
> >  
> >          bool "Rockchip generic power domain"
> >          depends on PM
> > 
> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> > index 3d73d06..c851fa0 100644
> > --- a/drivers/soc/rockchip/Makefile
> > +++ b/drivers/soc/rockchip/Makefile
> > @@ -1,4 +1,5 @@
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
> > 
> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> > 
> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> > new file mode 100644
> > index 0000000..0c85476a
> > --- /dev/null
> > +++ b/drivers/soc/rockchip/grf.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Rockchip Generic Register Files setup
> > + *
> > + * Copyright (c) 2016 Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +               ((val) << (shift) | (mask) << ((shift) + 16))
> > +
> > +struct rockchip_grf_value {
> > +       const char *desc;
> > +       u32 reg;
> > +       u32 val;
> > +};
> > +
> > +struct rockchip_grf_info {
> > +       const struct rockchip_grf_value *values;
> > +       int num_values;
> > +};
> > +
> > +#define RK3036_GRF_SOC_CON0            0x140
> > +
> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> > +       /*
> > +        * Disable auto jtag/sdmmc switching that causes issues with the
> > +        * clock-framework and the mmc controllers making them unreliable.
> > +        */
> > +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
> > +       .values = rk3036_defaults,
> > +       .num_values = ARRAY_SIZE(rk3036_defaults),
> > +};
> > +
> > +#define RK3288_GRF_SOC_CON0            0x244
> > +
> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> > +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
> > +       .values = rk3288_defaults,
> > +       .num_values = ARRAY_SIZE(rk3288_defaults),
> > +};
> > +
> > +#define RK3368_GRF_SOC_CON15           0x43c
> > +
> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> > +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13)
> > }, +};
> > +
> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
> > +       .values = rk3368_defaults,
> > +       .num_values = ARRAY_SIZE(rk3368_defaults),
> > +};
> > +
> > +#define RK3399_GRF_SOC_CON7            0xe21c
> > +
> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> > +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
> > +       .values = rk3399_defaults,
> > +       .num_values = ARRAY_SIZE(rk3399_defaults),
> > +};
> > +
> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> > +       {
> > +               .compatible = "rockchip,rk3036-grf",
> > +               .data = (void *)&rk3036_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3288-grf",
> > +               .data = (void *)&rk3288_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3368-grf",
> > +               .data = (void *)&rk3368_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3399-grf",
> > +               .data = (void *)&rk3399_grf,
> > +       },
> > +       { /* sentinel */ },
> > +};
> 
> I often come in after there's already been discussion on a topic, and
> don't always find all of it, so let me know if this has already been
> considered and not chosen for some reason:

So far only Doug had the time to look at this, so I'm happy about discussing 
my approach more.


> I get a little worried when you see these per-SoC tables build up in
> the kernel. It means there'll need to be additions here for a bunch of
> different SoCs.

We also add clock drivers (which are essentially only tables listing the 
clock-hirarchy) on a per-soc basis. In clk-land the move was the other way 
around, because it was deemed that defining clocks in the devicetree does not 
scale :-) .


> Would it be possible to describe this directly in the DT instead? If
> nothing else, something like
> 
> function-regs = < array of regs >
> function-values = < array of values >
> function-names = < array of names >
> 
> Not ideal either, especially if abused into a random poke table, but
> it won't require per-SoC changes to the kernel. If we keep the binding
> under close eyes we can hopefully avoid abuse.
> 
> Given that Rockchip is still working on new SoCs, this list will just
> grow and doing it in-kernel will stop scaling at some point.

As explained in the commit message, this is meant as a last resort for 
settings that no driver can be responsible for in a sensible way and that are 
Linux-specific and thus cannot be set in firmware for everybody. Most GRF-
specific settings are already done by the specific drivers needing them.

I.e. you see the mmc/jtag thing, which is getting disabled because the Linux 
mmc subsystem gets a hickup if the pinmux is changed by the soc to early after 
card eject. Other OSes might like that behaviour.

As the dts is meant to be for everyone I don't see how that could be sanely 
defined for things.


Another option would be to burden the rockchip mmc-driver with that specific 
setting, but then the mmc-driver (and possible other drivers if needed) would 
need per-soc additions, resulting in the same scaling problem.
Having one per-soc list at least keeps in one place :-) .


If you really dislike the current approach, we can of course postpone it for 
now though.


Heiko

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Olof,

Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > The General Register Files are an area of registers containing a lot
> > of single-bit settings for numerous components as well full components
> > like usbphy control. Therefore all used components are accessed
> > via the syscon provided by the grf nodes or from the sub-devices
> > created through the simple-mfd created from the grf node.
> > 
> > Some settings are not used by anything but will need to be set up
> > according to expectations on the kernel side.
> > 
> > Best example is the force_jtag setting, which defaults to on and
> > results in the soc switching the pin-outputs between jtag and sdmmc
> > automatically depending on the card-detect status. This conflicts
> > heavily with how the dw_mmc driver expects to do its work and also
> > with the clock-controller, which has most likely deactivated the
> > jtag clock due to it being unused.
> > 
> > So far the handling of this setting was living in the mach-rockchip
> > code for the arm32-based rk3288 but that of course doesn't work
> > for arm64 socs and would also look ugly for further arm32 socs.
> > 
> > Also always disabling this setting is quite specific to linux and
> > its subsystems, other operating systems might prefer other settings,
> > so that the bootloader cannot really set a sane default for all.
> > 
> > So introduce a top-level driver for the grf that handles these
> > settings that need to be a certain way but nobody cares about.
> > 
> > Other needed settings might surface in the future and can then
> > be added here, but only as a last option. Ideally general GRF
> > settings should be handled in the driver needing them.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/soc/rockchip/Kconfig  |  10 ++++
> >  drivers/soc/rockchip/Makefile |   1 +
> >  drivers/soc/rockchip/grf.c    | 134
> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
> >  insertions(+)
> >  create mode 100644 drivers/soc/rockchip/grf.c
> > 
> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> > index 7140ff8..20da55d 100644
> > --- a/drivers/soc/rockchip/Kconfig
> > +++ b/drivers/soc/rockchip/Kconfig
> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +
> > +config ROCKCHIP_GRF
> > +       bool
> > +       default y
> > +       help
> > +         The General Register Files are a central component providing
> > +         special additional settings registers for a lot of
> > soc-components. +         In a lot of cases there also need to be default
> > settings initialized +         to make some of them conform to
> > expectations of the kernel. +
> > 
> >  config ROCKCHIP_PM_DOMAINS
> >  
> >          bool "Rockchip generic power domain"
> >          depends on PM
> > 
> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> > index 3d73d06..c851fa0 100644
> > --- a/drivers/soc/rockchip/Makefile
> > +++ b/drivers/soc/rockchip/Makefile
> > @@ -1,4 +1,5 @@
> > 
> >  #
> >  # Rockchip Soc drivers
> >  #
> > 
> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
> > 
> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> > 
> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> > new file mode 100644
> > index 0000000..0c85476a
> > --- /dev/null
> > +++ b/drivers/soc/rockchip/grf.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Rockchip Generic Register Files setup
> > + *
> > + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +               ((val) << (shift) | (mask) << ((shift) + 16))
> > +
> > +struct rockchip_grf_value {
> > +       const char *desc;
> > +       u32 reg;
> > +       u32 val;
> > +};
> > +
> > +struct rockchip_grf_info {
> > +       const struct rockchip_grf_value *values;
> > +       int num_values;
> > +};
> > +
> > +#define RK3036_GRF_SOC_CON0            0x140
> > +
> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
> > +       /*
> > +        * Disable auto jtag/sdmmc switching that causes issues with the
> > +        * clock-framework and the mmc controllers making them unreliable.
> > +        */
> > +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
> > +       .values = rk3036_defaults,
> > +       .num_values = ARRAY_SIZE(rk3036_defaults),
> > +};
> > +
> > +#define RK3288_GRF_SOC_CON0            0x244
> > +
> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
> > +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
> > +       .values = rk3288_defaults,
> > +       .num_values = ARRAY_SIZE(rk3288_defaults),
> > +};
> > +
> > +#define RK3368_GRF_SOC_CON15           0x43c
> > +
> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
> > +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13)
> > }, +};
> > +
> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
> > +       .values = rk3368_defaults,
> > +       .num_values = ARRAY_SIZE(rk3368_defaults),
> > +};
> > +
> > +#define RK3399_GRF_SOC_CON7            0xe21c
> > +
> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
> > +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12)
> > },
> > +};
> > +
> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
> > +       .values = rk3399_defaults,
> > +       .num_values = ARRAY_SIZE(rk3399_defaults),
> > +};
> > +
> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
> > +       {
> > +               .compatible = "rockchip,rk3036-grf",
> > +               .data = (void *)&rk3036_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3288-grf",
> > +               .data = (void *)&rk3288_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3368-grf",
> > +               .data = (void *)&rk3368_grf,
> > +       }, {
> > +               .compatible = "rockchip,rk3399-grf",
> > +               .data = (void *)&rk3399_grf,
> > +       },
> > +       { /* sentinel */ },
> > +};
> 
> I often come in after there's already been discussion on a topic, and
> don't always find all of it, so let me know if this has already been
> considered and not chosen for some reason:

So far only Doug had the time to look at this, so I'm happy about discussing 
my approach more.


> I get a little worried when you see these per-SoC tables build up in
> the kernel. It means there'll need to be additions here for a bunch of
> different SoCs.

We also add clock drivers (which are essentially only tables listing the 
clock-hirarchy) on a per-soc basis. In clk-land the move was the other way 
around, because it was deemed that defining clocks in the devicetree does not 
scale :-) .


> Would it be possible to describe this directly in the DT instead? If
> nothing else, something like
> 
> function-regs = < array of regs >
> function-values = < array of values >
> function-names = < array of names >
> 
> Not ideal either, especially if abused into a random poke table, but
> it won't require per-SoC changes to the kernel. If we keep the binding
> under close eyes we can hopefully avoid abuse.
> 
> Given that Rockchip is still working on new SoCs, this list will just
> grow and doing it in-kernel will stop scaling at some point.

As explained in the commit message, this is meant as a last resort for 
settings that no driver can be responsible for in a sensible way and that are 
Linux-specific and thus cannot be set in firmware for everybody. Most GRF-
specific settings are already done by the specific drivers needing them.

I.e. you see the mmc/jtag thing, which is getting disabled because the Linux 
mmc subsystem gets a hickup if the pinmux is changed by the soc to early after 
card eject. Other OSes might like that behaviour.

As the dts is meant to be for everyone I don't see how that could be sanely 
defined for things.


Another option would be to burden the rockchip mmc-driver with that specific 
setting, but then the mmc-driver (and possible other drivers if needed) would 
need per-soc additions, resulting in the same scaling problem.
Having one per-soc list at least keeps in one place :-) .


If you really dislike the current approach, we can of course postpone it for 
now though.


Heiko

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Olof Johansson]

On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Olof,
>
> Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
>> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> > The General Register Files are an area of registers containing a lot
>> > of single-bit settings for numerous components as well full components
>> > like usbphy control. Therefore all used components are accessed
>> > via the syscon provided by the grf nodes or from the sub-devices
>> > created through the simple-mfd created from the grf node.
>> >
>> > Some settings are not used by anything but will need to be set up
>> > according to expectations on the kernel side.
>> >
>> > Best example is the force_jtag setting, which defaults to on and
>> > results in the soc switching the pin-outputs between jtag and sdmmc
>> > automatically depending on the card-detect status. This conflicts
>> > heavily with how the dw_mmc driver expects to do its work and also
>> > with the clock-controller, which has most likely deactivated the
>> > jtag clock due to it being unused.
>> >
>> > So far the handling of this setting was living in the mach-rockchip
>> > code for the arm32-based rk3288 but that of course doesn't work
>> > for arm64 socs and would also look ugly for further arm32 socs.
>> >
>> > Also always disabling this setting is quite specific to linux and
>> > its subsystems, other operating systems might prefer other settings,
>> > so that the bootloader cannot really set a sane default for all.
>> >
>> > So introduce a top-level driver for the grf that handles these
>> > settings that need to be a certain way but nobody cares about.
>> >
>> > Other needed settings might surface in the future and can then
>> > be added here, but only as a last option. Ideally general GRF
>> > settings should be handled in the driver needing them.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/soc/rockchip/Kconfig  |  10 ++++
>> >  drivers/soc/rockchip/Makefile |   1 +
>> >  drivers/soc/rockchip/grf.c    | 134
>> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
>> >  insertions(+)
>> >  create mode 100644 drivers/soc/rockchip/grf.c
>> >
>> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
>> > index 7140ff8..20da55d 100644
>> > --- a/drivers/soc/rockchip/Kconfig
>> > +++ b/drivers/soc/rockchip/Kconfig
>> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>> >
>> >  #
>> >  # Rockchip Soc drivers
>> >  #
>> >
>> > +
>> > +config ROCKCHIP_GRF
>> > +       bool
>> > +       default y
>> > +       help
>> > +         The General Register Files are a central component providing
>> > +         special additional settings registers for a lot of
>> > soc-components. +         In a lot of cases there also need to be default
>> > settings initialized +         to make some of them conform to
>> > expectations of the kernel. +
>> >
>> >  config ROCKCHIP_PM_DOMAINS
>> >
>> >          bool "Rockchip generic power domain"
>> >          depends on PM
>> >
>> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
>> > index 3d73d06..c851fa0 100644
>> > --- a/drivers/soc/rockchip/Makefile
>> > +++ b/drivers/soc/rockchip/Makefile
>> > @@ -1,4 +1,5 @@
>> >
>> >  #
>> >  # Rockchip Soc drivers
>> >  #
>> >
>> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>> >
>> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
>> >
>> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
>> > new file mode 100644
>> > index 0000000..0c85476a
>> > --- /dev/null
>> > +++ b/drivers/soc/rockchip/grf.c
>> > @@ -0,0 +1,134 @@
>> > +/*
>> > + * Rockchip Generic Register Files setup
>> > + *
>> > + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#include <linux/err.h>
>> > +#include <linux/regmap.h>
>> > +#include <linux/mfd/syscon.h>
>> > +#include <linux/of_device.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/regmap.h>
>> > +
>> > +#define HIWORD_UPDATE(val, mask, shift) \
>> > +               ((val) << (shift) | (mask) << ((shift) + 16))
>> > +
>> > +struct rockchip_grf_value {
>> > +       const char *desc;
>> > +       u32 reg;
>> > +       u32 val;
>> > +};
>> > +
>> > +struct rockchip_grf_info {
>> > +       const struct rockchip_grf_value *values;
>> > +       int num_values;
>> > +};
>> > +
>> > +#define RK3036_GRF_SOC_CON0            0x140
>> > +
>> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
>> > +       /*
>> > +        * Disable auto jtag/sdmmc switching that causes issues with the
>> > +        * clock-framework and the mmc controllers making them unreliable.
>> > +        */
>> > +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
>> > +       .values = rk3036_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3036_defaults),
>> > +};
>> > +
>> > +#define RK3288_GRF_SOC_CON0            0x244
>> > +
>> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
>> > +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
>> > +       .values = rk3288_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3288_defaults),
>> > +};
>> > +
>> > +#define RK3368_GRF_SOC_CON15           0x43c
>> > +
>> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
>> > +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13)
>> > }, +};
>> > +
>> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
>> > +       .values = rk3368_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3368_defaults),
>> > +};
>> > +
>> > +#define RK3399_GRF_SOC_CON7            0xe21c
>> > +
>> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
>> > +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
>> > +       .values = rk3399_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3399_defaults),
>> > +};
>> > +
>> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
>> > +       {
>> > +               .compatible = "rockchip,rk3036-grf",
>> > +               .data = (void *)&rk3036_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3288-grf",
>> > +               .data = (void *)&rk3288_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3368-grf",
>> > +               .data = (void *)&rk3368_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3399-grf",
>> > +               .data = (void *)&rk3399_grf,
>> > +       },
>> > +       { /* sentinel */ },
>> > +};
>>
>> I often come in after there's already been discussion on a topic, and
>> don't always find all of it, so let me know if this has already been
>> considered and not chosen for some reason:
>
> So far only Doug had the time to look at this, so I'm happy about discussing
> my approach more.
>
>
>> I get a little worried when you see these per-SoC tables build up in
>> the kernel. It means there'll need to be additions here for a bunch of
>> different SoCs.
>
> We also add clock drivers (which are essentially only tables listing the
> clock-hirarchy) on a per-soc basis. In clk-land the move was the other way
> around, because it was deemed that defining clocks in the devicetree does not
> scale :-) .

Yeah, I think the case of clocks is one where we've decided that it
makes sense to add a new clock driver per major SoC, too much changes
between each revision and doing it in DT (which some platforms have
attempted) isn't overall cleaner.

>> Would it be possible to describe this directly in the DT instead? If
>> nothing else, something like
>>
>> function-regs = < array of regs >
>> function-values = < array of values >
>> function-names = < array of names >
>>
>> Not ideal either, especially if abused into a random poke table, but
>> it won't require per-SoC changes to the kernel. If we keep the binding
>> under close eyes we can hopefully avoid abuse.
>>
>> Given that Rockchip is still working on new SoCs, this list will just
>> grow and doing it in-kernel will stop scaling at some point.
>
> As explained in the commit message, this is meant as a last resort for
> settings that no driver can be responsible for in a sensible way and that are
> Linux-specific and thus cannot be set in firmware for everybody. Most GRF-
> specific settings are already done by the specific drivers needing them.
>
> I.e. you see the mmc/jtag thing, which is getting disabled because the Linux
> mmc subsystem gets a hickup if the pinmux is changed by the soc to early after
> card eject. Other OSes might like that behaviour.
>
> As the dts is meant to be for everyone I don't see how that could be sanely
> defined for things.

But every OS that wants to use JTAG instead of MMC, or enable JTAG
around MMC, will need to know about this setting, right? So we're not
necessarily leaking linux implementation details into the DT here.
Whether each OS applies the setting is up to them.

> Another option would be to burden the rockchip mmc-driver with that specific
> setting, but then the mmc-driver (and possible other drivers if needed) would
> need per-soc additions, resulting in the same scaling problem.
> Having one per-soc list at least keeps in one place :-) .

That's exactly what I'm looking to avoid -- i.e. I don't want the GRF
driver to need patching for every SoC either, if we can avoid it.

Once the list gets too long, you'll want to add #ifdef SOCNAME and
only build in the tables for some of the SoCs. And then we've all lost
:-)

Or, if you're firmly deciding to keep updating kernel code for all
SoCs, could also just add one platform quirk file in
drivers/soc/rockchip with the postcore_initcall that matches toplevel
compatible per SoC, finds the device node, maps the address range,
sets the value.

That'll give you a place for other platform quirks, if they ever come
up. In one place, even if the other quirks might be in other register
ranges.

> If you really dislike the current approach, we can of course postpone it for
> now though.

It's a bit bikesheddy, but it just seems like adding a driver that
will always need updates is a bit of a heavy solution.


-Olof

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Olof Johansson]

On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Olof,
>
> Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
>> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> > The General Register Files are an area of registers containing a lot
>> > of single-bit settings for numerous components as well full components
>> > like usbphy control. Therefore all used components are accessed
>> > via the syscon provided by the grf nodes or from the sub-devices
>> > created through the simple-mfd created from the grf node.
>> >
>> > Some settings are not used by anything but will need to be set up
>> > according to expectations on the kernel side.
>> >
>> > Best example is the force_jtag setting, which defaults to on and
>> > results in the soc switching the pin-outputs between jtag and sdmmc
>> > automatically depending on the card-detect status. This conflicts
>> > heavily with how the dw_mmc driver expects to do its work and also
>> > with the clock-controller, which has most likely deactivated the
>> > jtag clock due to it being unused.
>> >
>> > So far the handling of this setting was living in the mach-rockchip
>> > code for the arm32-based rk3288 but that of course doesn't work
>> > for arm64 socs and would also look ugly for further arm32 socs.
>> >
>> > Also always disabling this setting is quite specific to linux and
>> > its subsystems, other operating systems might prefer other settings,
>> > so that the bootloader cannot really set a sane default for all.
>> >
>> > So introduce a top-level driver for the grf that handles these
>> > settings that need to be a certain way but nobody cares about.
>> >
>> > Other needed settings might surface in the future and can then
>> > be added here, but only as a last option. Ideally general GRF
>> > settings should be handled in the driver needing them.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/soc/rockchip/Kconfig  |  10 ++++
>> >  drivers/soc/rockchip/Makefile |   1 +
>> >  drivers/soc/rockchip/grf.c    | 134
>> >  ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145
>> >  insertions(+)
>> >  create mode 100644 drivers/soc/rockchip/grf.c
>> >
>> > diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
>> > index 7140ff8..20da55d 100644
>> > --- a/drivers/soc/rockchip/Kconfig
>> > +++ b/drivers/soc/rockchip/Kconfig
>> > @@ -3,6 +3,16 @@ if ARCH_ROCKCHIP || COMPILE_TEST
>> >
>> >  #
>> >  # Rockchip Soc drivers
>> >  #
>> >
>> > +
>> > +config ROCKCHIP_GRF
>> > +       bool
>> > +       default y
>> > +       help
>> > +         The General Register Files are a central component providing
>> > +         special additional settings registers for a lot of
>> > soc-components. +         In a lot of cases there also need to be default
>> > settings initialized +         to make some of them conform to
>> > expectations of the kernel. +
>> >
>> >  config ROCKCHIP_PM_DOMAINS
>> >
>> >          bool "Rockchip generic power domain"
>> >          depends on PM
>> >
>> > diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
>> > index 3d73d06..c851fa0 100644
>> > --- a/drivers/soc/rockchip/Makefile
>> > +++ b/drivers/soc/rockchip/Makefile
>> > @@ -1,4 +1,5 @@
>> >
>> >  #
>> >  # Rockchip Soc drivers
>> >  #
>> >
>> > +obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>> >
>> >  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
>> >
>> > diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
>> > new file mode 100644
>> > index 0000000..0c85476a
>> > --- /dev/null
>> > +++ b/drivers/soc/rockchip/grf.c
>> > @@ -0,0 +1,134 @@
>> > +/*
>> > + * Rockchip Generic Register Files setup
>> > + *
>> > + * Copyright (c) 2016 Heiko Stuebner <heiko@sntech.de>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#include <linux/err.h>
>> > +#include <linux/regmap.h>
>> > +#include <linux/mfd/syscon.h>
>> > +#include <linux/of_device.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/regmap.h>
>> > +
>> > +#define HIWORD_UPDATE(val, mask, shift) \
>> > +               ((val) << (shift) | (mask) << ((shift) + 16))
>> > +
>> > +struct rockchip_grf_value {
>> > +       const char *desc;
>> > +       u32 reg;
>> > +       u32 val;
>> > +};
>> > +
>> > +struct rockchip_grf_info {
>> > +       const struct rockchip_grf_value *values;
>> > +       int num_values;
>> > +};
>> > +
>> > +#define RK3036_GRF_SOC_CON0            0x140
>> > +
>> > +static const struct rockchip_grf_value rk3036_defaults[] __initconst = {
>> > +       /*
>> > +        * Disable auto jtag/sdmmc switching that causes issues with the
>> > +        * clock-framework and the mmc controllers making them unreliable.
>> > +        */
>> > +       { "jtag switching", RK3036_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 11)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3036_grf __initconst = {
>> > +       .values = rk3036_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3036_defaults),
>> > +};
>> > +
>> > +#define RK3288_GRF_SOC_CON0            0x244
>> > +
>> > +static const struct rockchip_grf_value rk3288_defaults[] __initconst = {
>> > +       { "jtag switching", RK3288_GRF_SOC_CON0, HIWORD_UPDATE(0, 1, 12)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3288_grf __initconst = {
>> > +       .values = rk3288_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3288_defaults),
>> > +};
>> > +
>> > +#define RK3368_GRF_SOC_CON15           0x43c
>> > +
>> > +static const struct rockchip_grf_value rk3368_defaults[] __initconst = {
>> > +       { "jtag switching", RK3368_GRF_SOC_CON15, HIWORD_UPDATE(0, 1, 13)
>> > }, +};
>> > +
>> > +static const struct rockchip_grf_info rk3368_grf __initconst = {
>> > +       .values = rk3368_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3368_defaults),
>> > +};
>> > +
>> > +#define RK3399_GRF_SOC_CON7            0xe21c
>> > +
>> > +static const struct rockchip_grf_value rk3399_defaults[] __initconst = {
>> > +       { "jtag switching", RK3399_GRF_SOC_CON7, HIWORD_UPDATE(0, 1, 12)
>> > },
>> > +};
>> > +
>> > +static const struct rockchip_grf_info rk3399_grf __initconst = {
>> > +       .values = rk3399_defaults,
>> > +       .num_values = ARRAY_SIZE(rk3399_defaults),
>> > +};
>> > +
>> > +static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
>> > +       {
>> > +               .compatible = "rockchip,rk3036-grf",
>> > +               .data = (void *)&rk3036_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3288-grf",
>> > +               .data = (void *)&rk3288_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3368-grf",
>> > +               .data = (void *)&rk3368_grf,
>> > +       }, {
>> > +               .compatible = "rockchip,rk3399-grf",
>> > +               .data = (void *)&rk3399_grf,
>> > +       },
>> > +       { /* sentinel */ },
>> > +};
>>
>> I often come in after there's already been discussion on a topic, and
>> don't always find all of it, so let me know if this has already been
>> considered and not chosen for some reason:
>
> So far only Doug had the time to look at this, so I'm happy about discussing
> my approach more.
>
>
>> I get a little worried when you see these per-SoC tables build up in
>> the kernel. It means there'll need to be additions here for a bunch of
>> different SoCs.
>
> We also add clock drivers (which are essentially only tables listing the
> clock-hirarchy) on a per-soc basis. In clk-land the move was the other way
> around, because it was deemed that defining clocks in the devicetree does not
> scale :-) .

Yeah, I think the case of clocks is one where we've decided that it
makes sense to add a new clock driver per major SoC, too much changes
between each revision and doing it in DT (which some platforms have
attempted) isn't overall cleaner.

>> Would it be possible to describe this directly in the DT instead? If
>> nothing else, something like
>>
>> function-regs = < array of regs >
>> function-values = < array of values >
>> function-names = < array of names >
>>
>> Not ideal either, especially if abused into a random poke table, but
>> it won't require per-SoC changes to the kernel. If we keep the binding
>> under close eyes we can hopefully avoid abuse.
>>
>> Given that Rockchip is still working on new SoCs, this list will just
>> grow and doing it in-kernel will stop scaling at some point.
>
> As explained in the commit message, this is meant as a last resort for
> settings that no driver can be responsible for in a sensible way and that are
> Linux-specific and thus cannot be set in firmware for everybody. Most GRF-
> specific settings are already done by the specific drivers needing them.
>
> I.e. you see the mmc/jtag thing, which is getting disabled because the Linux
> mmc subsystem gets a hickup if the pinmux is changed by the soc to early after
> card eject. Other OSes might like that behaviour.
>
> As the dts is meant to be for everyone I don't see how that could be sanely
> defined for things.

But every OS that wants to use JTAG instead of MMC, or enable JTAG
around MMC, will need to know about this setting, right? So we're not
necessarily leaking linux implementation details into the DT here.
Whether each OS applies the setting is up to them.

> Another option would be to burden the rockchip mmc-driver with that specific
> setting, but then the mmc-driver (and possible other drivers if needed) would
> need per-soc additions, resulting in the same scaling problem.
> Having one per-soc list at least keeps in one place :-) .

That's exactly what I'm looking to avoid -- i.e. I don't want the GRF
driver to need patching for every SoC either, if we can avoid it.

Once the list gets too long, you'll want to add #ifdef SOCNAME and
only build in the tables for some of the SoCs. And then we've all lost
:-)

Or, if you're firmly deciding to keep updating kernel code for all
SoCs, could also just add one platform quirk file in
drivers/soc/rockchip with the postcore_initcall that matches toplevel
compatible per SoC, finds the device node, maps the address range,
sets the value.

That'll give you a place for other platform quirks, if they ever come
up. In one place, even if the other quirks might be in other register
ranges.

> If you really dislike the current approach, we can of course postpone it for
> now though.

It's a bit bikesheddy, but it just seems like adding a driver that
will always need updates is a bit of a heavy solution.


-Olof

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Olof,

sorry that took way longer than I expected, I was slightly swamped the last 
days with general work and the travel to Portland.

Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
> >> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:

[...]

> >> Would it be possible to describe this directly in the DT instead? If
> >> nothing else, something like
> >> 
> >> function-regs = < array of regs >
> >> function-values = < array of values >
> >> function-names = < array of names >
> >> 
> >> Not ideal either, especially if abused into a random poke table, but
> >> it won't require per-SoC changes to the kernel. If we keep the binding
> >> under close eyes we can hopefully avoid abuse.
> >> 
> >> Given that Rockchip is still working on new SoCs, this list will just
> >> grow and doing it in-kernel will stop scaling at some point.
> > 
> > As explained in the commit message, this is meant as a last resort for
> > settings that no driver can be responsible for in a sensible way and that
> > are Linux-specific and thus cannot be set in firmware for everybody. Most
> > GRF- specific settings are already done by the specific drivers needing
> > them.
> > 
> > I.e. you see the mmc/jtag thing, which is getting disabled because the
> > Linux mmc subsystem gets a hickup if the pinmux is changed by the soc to
> > early after card eject. Other OSes might like that behaviour.
> > 
> > As the dts is meant to be for everyone I don't see how that could be
> > sanely
> > defined for things.
> 
> But every OS that wants to use JTAG instead of MMC, or enable JTAG
> around MMC, will need to know about this setting, right? So we're not
> necessarily leaking linux implementation details into the DT here.
> Whether each OS applies the setting is up to them.

The issue with this specific setting is that the soc wants to be more 
intelligent than the kernel :-)
It makes the soc switch pin-muxes automatically between mmc and jtag depending 
on the card-detect status - circumventing pinctrl completely and killing the 
our mmc subsystem in the process.

You can of course still select jtag pinmuxes using the regular linux pinctrl 
subsystem. But as I said, other systems might like that functionality for 
whatever reason.


Describing the value (and others) in the dt creates the issue, that the driver 
code still needs to have a list of "we as Linux want to set the jtag-property 
but not other-property-y" and so on and would therefore rely on the function 
being stable over multiple socs, as otherwise we may end up with function-
names "mmc-jtag-rk3288", ... as the GRF and its contents are notorious 
unpredictable :-) .


> > Another option would be to burden the rockchip mmc-driver with that
> > specific setting, but then the mmc-driver (and possible other drivers if
> > needed) would need per-soc additions, resulting in the same scaling
> > problem.
> > Having one per-soc list at least keeps in one place :-) .
> 
> That's exactly what I'm looking to avoid -- i.e. I don't want the GRF
> driver to need patching for every SoC either, if we can avoid it.
> 
> Once the list gets too long, you'll want to add #ifdef SOCNAME and
> only build in the tables for some of the SoCs. And then we've all lost
> 
> :-)

Right now we don't have any SOCNAME stuff. It's in the back of my mind because 
we're currently also compiling for example all clock drivers for all 
ARCH_ROCKCHIP socs, so it might make sense to split that up a bit, so people 
will be able to limit kernel size if they like to.

But one thing I stumbled upon is that while some arm32 socs do have per-soc 
kconfig entries, there does not seem to be anything like this on arm64 yet.

Is there a plan stated somewhere already on how this should be handled?
I.e. introducing SOC_RK3399 (or similar) or having separate CLK_RK3399, 
GRF_RK3399 (or similar) symbols that all need to be disabled separately if 
desired.

As stated in the original commit message, the list of settings in there is not 
supposed to get "long", but that may or may not be wishful thinking on my part 
;-) .

> Or, if you're firmly deciding to keep updating kernel code for all
> SoCs, could also just add one platform quirk file in
> drivers/soc/rockchip with the postcore_initcall that matches toplevel
> compatible per SoC, finds the device node, maps the address range,
> sets the value.
> 
> That'll give you a place for other platform quirks, if they ever come
> up. In one place, even if the other quirks might be in other register
> ranges.

I would very much prefer going this route, as I really don't trust the GRF to 
keep conforming to any devicetree binding (or anything at all) and this way, 
stuff is completely reversable / redoable if the need arises (hopefully not) 
and we don't have to live with an inadequate bindings forever :-) .


> > If you really dislike the current approach, we can of course postpone it
> > for now though.
> 
> It's a bit bikesheddy, but it just seems like adding a driver that
> will always need updates is a bit of a heavy solution.

It's not the only one though, needing per soc updates.

Closed neighbour is probably the io-domain handling in [0], whose binding I 
really like, as it hides the the GRF-shenanigans nicely and only exposes the 
actual supplies.

So yes, I'd like to keep it codified, but we can split that up on a per-soc 
basis as you suggested.


Heiko


[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
drivers/power/avs/rockchip-io-domain.c

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Heiko Stuebner]

Hi Olof,

sorry that took way longer than I expected, I was slightly swamped the last 
days with general work and the travel to Portland.

Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Am Sonntag, 29. Januar 2017, 14:40:53 CET schrieb Olof Johansson:
> >> On Tue, Nov 15, 2016 at 2:38 PM, Heiko Stuebner <heiko@sntech.de> wrote:

[...]

> >> Would it be possible to describe this directly in the DT instead? If
> >> nothing else, something like
> >> 
> >> function-regs = < array of regs >
> >> function-values = < array of values >
> >> function-names = < array of names >
> >> 
> >> Not ideal either, especially if abused into a random poke table, but
> >> it won't require per-SoC changes to the kernel. If we keep the binding
> >> under close eyes we can hopefully avoid abuse.
> >> 
> >> Given that Rockchip is still working on new SoCs, this list will just
> >> grow and doing it in-kernel will stop scaling at some point.
> > 
> > As explained in the commit message, this is meant as a last resort for
> > settings that no driver can be responsible for in a sensible way and that
> > are Linux-specific and thus cannot be set in firmware for everybody. Most
> > GRF- specific settings are already done by the specific drivers needing
> > them.
> > 
> > I.e. you see the mmc/jtag thing, which is getting disabled because the
> > Linux mmc subsystem gets a hickup if the pinmux is changed by the soc to
> > early after card eject. Other OSes might like that behaviour.
> > 
> > As the dts is meant to be for everyone I don't see how that could be
> > sanely
> > defined for things.
> 
> But every OS that wants to use JTAG instead of MMC, or enable JTAG
> around MMC, will need to know about this setting, right? So we're not
> necessarily leaking linux implementation details into the DT here.
> Whether each OS applies the setting is up to them.

The issue with this specific setting is that the soc wants to be more 
intelligent than the kernel :-)
It makes the soc switch pin-muxes automatically between mmc and jtag depending 
on the card-detect status - circumventing pinctrl completely and killing the 
our mmc subsystem in the process.

You can of course still select jtag pinmuxes using the regular linux pinctrl 
subsystem. But as I said, other systems might like that functionality for 
whatever reason.


Describing the value (and others) in the dt creates the issue, that the driver 
code still needs to have a list of "we as Linux want to set the jtag-property 
but not other-property-y" and so on and would therefore rely on the function 
being stable over multiple socs, as otherwise we may end up with function-
names "mmc-jtag-rk3288", ... as the GRF and its contents are notorious 
unpredictable :-) .


> > Another option would be to burden the rockchip mmc-driver with that
> > specific setting, but then the mmc-driver (and possible other drivers if
> > needed) would need per-soc additions, resulting in the same scaling
> > problem.
> > Having one per-soc list at least keeps in one place :-) .
> 
> That's exactly what I'm looking to avoid -- i.e. I don't want the GRF
> driver to need patching for every SoC either, if we can avoid it.
> 
> Once the list gets too long, you'll want to add #ifdef SOCNAME and
> only build in the tables for some of the SoCs. And then we've all lost
> 
> :-)

Right now we don't have any SOCNAME stuff. It's in the back of my mind because 
we're currently also compiling for example all clock drivers for all 
ARCH_ROCKCHIP socs, so it might make sense to split that up a bit, so people 
will be able to limit kernel size if they like to.

But one thing I stumbled upon is that while some arm32 socs do have per-soc 
kconfig entries, there does not seem to be anything like this on arm64 yet.

Is there a plan stated somewhere already on how this should be handled?
I.e. introducing SOC_RK3399 (or similar) or having separate CLK_RK3399, 
GRF_RK3399 (or similar) symbols that all need to be disabled separately if 
desired.

As stated in the original commit message, the list of settings in there is not 
supposed to get "long", but that may or may not be wishful thinking on my part 
;-) .

> Or, if you're firmly deciding to keep updating kernel code for all
> SoCs, could also just add one platform quirk file in
> drivers/soc/rockchip with the postcore_initcall that matches toplevel
> compatible per SoC, finds the device node, maps the address range,
> sets the value.
> 
> That'll give you a place for other platform quirks, if they ever come
> up. In one place, even if the other quirks might be in other register
> ranges.

I would very much prefer going this route, as I really don't trust the GRF to 
keep conforming to any devicetree binding (or anything at all) and this way, 
stuff is completely reversable / redoable if the need arises (hopefully not) 
and we don't have to live with an inadequate bindings forever :-) .


> > If you really dislike the current approach, we can of course postpone it
> > for now though.
> 
> It's a bit bikesheddy, but it just seems like adding a driver that
> will always need updates is a bit of a heavy solution.

It's not the only one though, needing per soc updates.

Closed neighbour is probably the io-domain handling in [0], whose binding I 
really like, as it hides the the GRF-shenanigans nicely and only exposes the 
actual supplies.

So yes, I'd like to keep it codified, but we can split that up on a per-soc 
basis as you suggested.


Heiko


[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
drivers/power/avs/rockchip-io-domain.c

Re: [PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Brian Norris]

To throw my unwanted 2-cents in:

On Mon, Feb 27, 2017 at 06:30:08AM +0100, Heiko Stuebner wrote:
> Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> > On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Or, if you're firmly deciding to keep updating kernel code for all
> > SoCs, could also just add one platform quirk file in
> > drivers/soc/rockchip with the postcore_initcall that matches toplevel
> > compatible per SoC, finds the device node, maps the address range,
> > sets the value.
> > 
> > That'll give you a place for other platform quirks, if they ever come
> > up. In one place, even if the other quirks might be in other register
> > ranges.
> 
> I would very much prefer going this route, as I really don't trust the GRF to 
> keep conforming to any devicetree binding (or anything at all) and this way, 
> stuff is completely reversable / redoable if the need arises (hopefully not) 
> and we don't have to live with an inadequate bindings forever :-) .

Agreed, FWIW (though I'm not sure why a top-level chip driver is better
than a GRF driver). Rockchip's GRF is basically "designed" (that's
probably not an accurate word) to need updating on every chip revision.
I'm also not very confident in any given binding around it. DT bindings
are notoriously hard to modify after they've been accepted, and I don't
see much actual value in foolishly codifying every bit of GRF in the
device tree bindings.

I think this pattern applies elsewhere too -- for instance, on the
Type-C PHY controller [1], which currently defies the above and instead
tries to list each GRF offset it needs in the DT; this is, IMO, quite
unscalable. We're already nearly breaking backward compatibility, and
we've only supported one SoC there!!

Brian

[1] https://patchwork.kernel.org/patch/9566079/
    [PATCH 3/4] phy: rockchip-typec: support DP phy switch

[PATCH v2 2/3] soc: rockchip: add driver handling grf setup

[Brian Norris]

To throw my unwanted 2-cents in:

On Mon, Feb 27, 2017 at 06:30:08AM +0100, Heiko Stuebner wrote:
> Am Sonntag, 29. Januar 2017, 16:41:46 CET schrieb Olof Johansson:
> > On Sun, Jan 29, 2017 at 3:36 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Or, if you're firmly deciding to keep updating kernel code for all
> > SoCs, could also just add one platform quirk file in
> > drivers/soc/rockchip with the postcore_initcall that matches toplevel
> > compatible per SoC, finds the device node, maps the address range,
> > sets the value.
> > 
> > That'll give you a place for other platform quirks, if they ever come
> > up. In one place, even if the other quirks might be in other register
> > ranges.
> 
> I would very much prefer going this route, as I really don't trust the GRF to 
> keep conforming to any devicetree binding (or anything at all) and this way, 
> stuff is completely reversable / redoable if the need arises (hopefully not) 
> and we don't have to live with an inadequate bindings forever :-) .

Agreed, FWIW (though I'm not sure why a top-level chip driver is better
than a GRF driver). Rockchip's GRF is basically "designed" (that's
probably not an accurate word) to need updating on every chip revision.
I'm also not very confident in any given binding around it. DT bindings
are notoriously hard to modify after they've been accepted, and I don't
see much actual value in foolishly codifying every bit of GRF in the
device tree bindings.

I think this pattern applies elsewhere too -- for instance, on the
Type-C PHY controller [1], which currently defies the above and instead
tries to list each GRF offset it needs in the DT; this is, IMO, quite
unscalable. We're already nearly breaking backward compatibility, and
we've only supported one SoC there!!

Brian

[1] https://patchwork.kernel.org/patch/9566079/
    [PATCH 3/4] phy: rockchip-typec: support DP phy switch