* [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-21 9:46 ` Sascha Hauer
2022-09-19 11:39 ` [PATCH v2 2/7] arm: rockchip_v8_defconfig: enable io domain driver Michael Riesch
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
The IO domains in Rockchip SoCs need to be configured to match the
corresponding bank voltage. In Linux this is achieved by means of a
platform driver that reads the voltage value of the supplies and
configures the bits in the general register file (GRF) accordingly.
Port this driver to barebox to provide support for the Rockchip
RK356x SoCs.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/rockchip/Kconfig | 17 +++
drivers/soc/rockchip/Makefile | 6 +
drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
5 files changed, 248 insertions(+)
create mode 100644 drivers/soc/rockchip/Kconfig
create mode 100644 drivers/soc/rockchip/Makefile
create mode 100644 drivers/soc/rockchip/io-domain.c
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 54b69cc42e..c0fe214429 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -2,5 +2,6 @@ menu "SoC drivers"
source "drivers/soc/imx/Kconfig"
source "drivers/soc/kvx/Kconfig"
+source "drivers/soc/rockchip/Kconfig"
endmenu
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index a23e81ffb3..9bff737b78 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -2,5 +2,6 @@
obj-$(CONFIG_ARCH_IMX) += imx/
obj-$(CONFIG_KVX) += kvx/
+obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/
obj-$(CONFIG_CPU_SIFIVE) += sifive/
obj-$(CONFIG_SOC_STARFIVE) += starfive/
diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
new file mode 100644
index 0000000000..3484b37e22
--- /dev/null
+++ b/drivers/soc/rockchip/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+if ARCH_ROCKCHIP || COMPILE_TEST
+
+menu "Rockchip SoC drivers"
+
+config ROCKCHIP_IODOMAIN
+ tristate "Rockchip IO domain support"
+ depends on OFDEVICE
+ help
+ Say y here to enable support io domains on Rockchip SoCs. It is
+ necessary for the io domain setting of the SoC to match the
+ voltage supplied by the regulators.
+
+endmenu
+
+endif
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
new file mode 100644
index 0000000000..104fad968e
--- /dev/null
+++ b/drivers/soc/rockchip/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Rockchip Soc drivers
+#
+
+obj-$(CONFIG_ROCKCHIP_IODOMAIN) += io-domain.o
diff --git a/drivers/soc/rockchip/io-domain.c b/drivers/soc/rockchip/io-domain.c
new file mode 100644
index 0000000000..ac5724c0e7
--- /dev/null
+++ b/drivers/soc/rockchip/io-domain.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Rockchip IO Voltage Domain driver
+ *
+ * Copyright 2014 MundoReader S.L.
+ * Copyright 2014 Google, Inc.
+ */
+
+#include <common.h>
+#include <driver.h>
+#include <errno.h>
+#include <init.h>
+#include <io.h>
+#include <of.h>
+#include <linux/err.h>
+#include <mfd/syscon.h>
+#include <regmap.h>
+#include <regulator.h>
+
+#define MAX_SUPPLIES 16
+
+/*
+ * The max voltage for 1.8V and 3.3V come from the Rockchip datasheet under
+ * "Recommended Operating Conditions" for "Digital GPIO". When the typical
+ * is 3.3V the max is 3.6V. When the typical is 1.8V the max is 1.98V.
+ *
+ * They are used like this:
+ * - If the voltage on a rail is above the "1.8" voltage (1.98V) we'll tell the
+ * SoC we're at 3.3.
+ * - If the voltage on a rail is above the "3.3" voltage (3.6V) we'll consider
+ * that to be an error.
+ */
+#define MAX_VOLTAGE_1_8 1980000
+#define MAX_VOLTAGE_3_3 3600000
+
+#define RK3568_PMU_GRF_IO_VSEL0 (0x0140)
+#define RK3568_PMU_GRF_IO_VSEL1 (0x0144)
+#define RK3568_PMU_GRF_IO_VSEL2 (0x0148)
+
+struct rockchip_iodomain;
+
+struct rockchip_iodomain_supply {
+ struct rockchip_iodomain *iod;
+ struct regulator *reg;
+ int idx;
+};
+
+struct rockchip_iodomain_soc_data {
+ int grf_offset;
+ const char *supply_names[MAX_SUPPLIES];
+ void (*init)(struct rockchip_iodomain *iod);
+ int (*write)(struct rockchip_iodomain_supply *supply, int uV);
+};
+
+struct rockchip_iodomain {
+ struct device_d *dev;
+ struct regmap *grf;
+ const struct rockchip_iodomain_soc_data *soc_data;
+ struct rockchip_iodomain_supply supplies[MAX_SUPPLIES];
+ int (*write)(struct rockchip_iodomain_supply *supply, int uV);
+};
+
+static int rk3568_iodomain_write(struct rockchip_iodomain_supply *supply,
+ int uV)
+{
+ struct rockchip_iodomain *iod = supply->iod;
+ u32 is_3v3 = uV > MAX_VOLTAGE_1_8;
+ u32 val0, val1;
+ int b;
+
+ dev_dbg(iod->dev, "set domain %d to %d uV\n", supply->idx, uV);
+
+ switch (supply->idx) {
+ case 0: /* pmuio1 */
+ break;
+ case 1: /* pmuio2 */
+ b = supply->idx;
+ val0 = BIT(16 + b) | (is_3v3 ? 0 : BIT(b));
+ b = supply->idx + 4;
+ val1 = BIT(16 + b) | (is_3v3 ? BIT(b) : 0);
+
+ regmap_write(iod->grf, RK3568_PMU_GRF_IO_VSEL2, val0);
+ regmap_write(iod->grf, RK3568_PMU_GRF_IO_VSEL2, val1);
+ break;
+ case 3: /* vccio2 */
+ break;
+ case 2: /* vccio1 */
+ case 4: /* vccio3 */
+ case 5: /* vccio4 */
+ case 6: /* vccio5 */
+ case 7: /* vccio6 */
+ case 8: /* vccio7 */
+ b = supply->idx - 1;
+ val0 = BIT(16 + b) | (is_3v3 ? 0 : BIT(b));
+ val1 = BIT(16 + b) | (is_3v3 ? BIT(b) : 0);
+
+ regmap_write(iod->grf, RK3568_PMU_GRF_IO_VSEL0, val0);
+ regmap_write(iod->grf, RK3568_PMU_GRF_IO_VSEL1, val1);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct rockchip_iodomain_soc_data soc_data_rk3568_pmu = {
+ .grf_offset = 0x140,
+ .supply_names = {
+ "pmuio1",
+ "pmuio2",
+ "vccio1",
+ "vccio2",
+ "vccio3",
+ "vccio4",
+ "vccio5",
+ "vccio6",
+ "vccio7",
+ },
+ .write = rk3568_iodomain_write,
+};
+
+static const struct of_device_id rockchip_iodomain_match[] = {
+ { .compatible = "rockchip,rk3568-pmu-io-voltage-domain",
+ .data = &soc_data_rk3568_pmu },
+ { /* sentinel */ },
+};
+
+static int rockchip_iodomain_probe(struct device_d *dev)
+{
+ struct device_node *np = dev->device_node, *parent;
+ struct rockchip_iodomain *iod;
+ int i, ret = 0;
+
+ if (!np)
+ return -ENODEV;
+
+ iod = xzalloc(sizeof(*iod));
+ iod->dev = dev;
+ iod->soc_data = device_get_match_data(dev);
+
+ if (iod->soc_data->write)
+ iod->write = iod->soc_data->write;
+
+ parent = of_get_parent(np);
+ if (parent) {
+ iod->grf = syscon_node_to_regmap(parent);
+ } else {
+ dev_dbg(dev, "falling back to old binding\n");
+ iod->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
+ }
+
+ if (IS_ERR(iod->grf)) {
+ dev_err(dev, "couldn't find grf regmap\n");
+ return PTR_ERR(iod->grf);
+ }
+
+ for (i = 0; i < MAX_SUPPLIES; i++) {
+ const char *supply_name = iod->soc_data->supply_names[i];
+ struct rockchip_iodomain_supply *supply = &iod->supplies[i];
+ struct regulator *reg;
+ int uV;
+
+ if (!supply_name)
+ continue;
+
+ reg = regulator_get(dev, supply_name);
+ if (IS_ERR(reg)) {
+ ret = PTR_ERR(reg);
+
+ /* If a supply wasn't specified, that's OK */
+ if (ret == -ENODEV)
+ continue;
+ else if (ret != -EPROBE_DEFER)
+ dev_err(dev, "couldn't get regulator %s\n",
+ supply_name);
+ goto out;
+ }
+
+ /* set initial correct value */
+ uV = regulator_get_voltage(reg);
+
+ /* must be a regulator we can get the voltage of */
+ if (uV < 0) {
+ dev_err(dev, "Can't determine voltage: %s\n",
+ supply_name);
+ ret = uV;
+ goto out;
+ }
+
+ if (uV > MAX_VOLTAGE_3_3) {
+ dev_crit(dev, "%d uV is too high. May damage SoC!\n",
+ uV);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* setup our supply */
+ supply->idx = i;
+ supply->iod = iod;
+ supply->reg = reg;
+
+ ret = iod->write(supply, uV);
+ if (ret) {
+ supply->reg = NULL;
+ goto out;
+ }
+ }
+
+ if (iod->soc_data->init)
+ iod->soc_data->init(iod);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+static struct driver_d rockchip_iodomain_driver = {
+ .name = "rockchip-iodomain",
+ .probe = rockchip_iodomain_probe,
+ .of_compatible = rockchip_iodomain_match,
+};
+coredevice_platform_driver(rockchip_iodomain_driver);
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-19 11:39 ` [PATCH v2 1/7] " Michael Riesch
@ 2022-09-21 9:46 ` Sascha Hauer
2022-09-21 11:49 ` Michael Riesch
0 siblings, 1 reply; 17+ messages in thread
From: Sascha Hauer @ 2022-09-21 9:46 UTC (permalink / raw)
To: Michael Riesch; +Cc: barebox, Frank Wunderlich
On Mon, Sep 19, 2022 at 01:39:42PM +0200, Michael Riesch wrote:
> The IO domains in Rockchip SoCs need to be configured to match the
> corresponding bank voltage. In Linux this is achieved by means of a
> platform driver that reads the voltage value of the supplies and
> configures the bits in the general register file (GRF) accordingly.
> Port this driver to barebox to provide support for the Rockchip
> RK356x SoCs.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/rockchip/Kconfig | 17 +++
> drivers/soc/rockchip/Makefile | 6 +
> drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
> 5 files changed, 248 insertions(+)
> create mode 100644 drivers/soc/rockchip/Kconfig
> create mode 100644 drivers/soc/rockchip/Makefile
> create mode 100644 drivers/soc/rockchip/io-domain.c
There's nothing in this driver that makes sure it is probed before the
users of the io domains. What happens when the users are probed before
the io domain driver?
Normally I would expect that the consumers have some phandle to their io
domain, but there's nothing like it in the device trees.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-21 9:46 ` Sascha Hauer
@ 2022-09-21 11:49 ` Michael Riesch
2022-09-22 8:03 ` Sascha Hauer
0 siblings, 1 reply; 17+ messages in thread
From: Michael Riesch @ 2022-09-21 11:49 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox, Frank Wunderlich
Hi Sascha,
On 9/21/22 11:46, Sascha Hauer wrote:
> On Mon, Sep 19, 2022 at 01:39:42PM +0200, Michael Riesch wrote:
>> The IO domains in Rockchip SoCs need to be configured to match the
>> corresponding bank voltage. In Linux this is achieved by means of a
>> platform driver that reads the voltage value of the supplies and
>> configures the bits in the general register file (GRF) accordingly.
>> Port this driver to barebox to provide support for the Rockchip
>> RK356x SoCs.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>> drivers/soc/Kconfig | 1 +
>> drivers/soc/Makefile | 1 +
>> drivers/soc/rockchip/Kconfig | 17 +++
>> drivers/soc/rockchip/Makefile | 6 +
>> drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
>> 5 files changed, 248 insertions(+)
>> create mode 100644 drivers/soc/rockchip/Kconfig
>> create mode 100644 drivers/soc/rockchip/Makefile
>> create mode 100644 drivers/soc/rockchip/io-domain.c
>
> There's nothing in this driver that makes sure it is probed before the
> users of the io domains. What happens when the users are probed before
> the io domain driver?
Correct. Unfortunately, the situation is quite the same in the kernel.
Only recently there was a discussion as to how to resolve this issue
[0]. While the RFC in [0] was received well there is no mainline
solution to this problem. I would suggest that the mainline kernel
solution is adopted once there is one.
In the mean time we could
- accept that it just works for some reason (TM)
- leave the magic bits in the low-level initialization to be on the
safe side
- simply wait
- ???
> Normally I would expect that the consumers have some phandle to their io
> domain, but there's nothing like it in the device trees.
The RFC in [0] suggests that the pinctrl is a consumer of the io
domains, which would be in alignment with your expectations.
Best regards,
Michael
[0]
https://lore.kernel.org/lkml/20220802095252.2486591-1-foss+kernel@0leil.net/T/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-21 11:49 ` Michael Riesch
@ 2022-09-22 8:03 ` Sascha Hauer
2022-09-22 8:13 ` Ahmad Fatoum
0 siblings, 1 reply; 17+ messages in thread
From: Sascha Hauer @ 2022-09-22 8:03 UTC (permalink / raw)
To: Michael Riesch; +Cc: barebox, Frank Wunderlich
On Wed, Sep 21, 2022 at 01:49:49PM +0200, Michael Riesch wrote:
> Hi Sascha,
>
> On 9/21/22 11:46, Sascha Hauer wrote:
> > On Mon, Sep 19, 2022 at 01:39:42PM +0200, Michael Riesch wrote:
> >> The IO domains in Rockchip SoCs need to be configured to match the
> >> corresponding bank voltage. In Linux this is achieved by means of a
> >> platform driver that reads the voltage value of the supplies and
> >> configures the bits in the general register file (GRF) accordingly.
> >> Port this driver to barebox to provide support for the Rockchip
> >> RK356x SoCs.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >> ---
> >> drivers/soc/Kconfig | 1 +
> >> drivers/soc/Makefile | 1 +
> >> drivers/soc/rockchip/Kconfig | 17 +++
> >> drivers/soc/rockchip/Makefile | 6 +
> >> drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
> >> 5 files changed, 248 insertions(+)
> >> create mode 100644 drivers/soc/rockchip/Kconfig
> >> create mode 100644 drivers/soc/rockchip/Makefile
> >> create mode 100644 drivers/soc/rockchip/io-domain.c
> >
> > There's nothing in this driver that makes sure it is probed before the
> > users of the io domains. What happens when the users are probed before
> > the io domain driver?
>
> Correct. Unfortunately, the situation is quite the same in the kernel.
> Only recently there was a discussion as to how to resolve this issue
> [0]. While the RFC in [0] was received well there is no mainline
> solution to this problem. I would suggest that the mainline kernel
> solution is adopted once there is one.
>
> In the mean time we could
> - accept that it just works for some reason (TM)
> - leave the magic bits in the low-level initialization to be on the
> safe side
Decided for this solution for now.
I added the following as a reminder that there's still something
to do.
Sascha
---------------------------8<---------------------------
>From 502c4e819b80a6ccaefbce4af4c55b830b7e2c73 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Thu, 22 Sep 2022 09:56:57 +0200
Subject: [PATCH] ARM: Rockchip: Add FIXME comment to io domain setup
We have a io-domain driver for rockchip boards which correctly
configures the io domain voltages, but currently there is no way
to make sure the io-domain driver is probed before its consumers.
To be on the safe side keep the io domain setup in the lowlevel code
for now, but add a comment that it should be removed once this issue
is resolved.
To solve this issue we need a phandle from the consumers to the io
domain node. We could add this in barebox locally, but decided to
wait until the upstream dts files have them.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
arch/arm/boards/radxa-rock3/lowlevel.c | 4 ++++
arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c | 4 ++++
arch/arm/boards/rockchip-rk3568-evb/lowlevel.c | 4 ++++
3 files changed, 12 insertions(+)
diff --git a/arch/arm/boards/radxa-rock3/lowlevel.c b/arch/arm/boards/radxa-rock3/lowlevel.c
index 2a449c17ae..a62f60dff8 100644
--- a/arch/arm/boards/radxa-rock3/lowlevel.c
+++ b/arch/arm/boards/radxa-rock3/lowlevel.c
@@ -17,6 +17,10 @@ static noinline void rk3568_start(void)
/*
* Enable vccio4 1.8V and vccio6 1.8V
* Needed for GMAC to work.
+ * FIXME: This is done by the io-domain driver as well, but there
+ * currently is no mechanism to make sure the driver gets probed
+ * before its consumers. Remove this setup once this issue is
+ * resolved.
*/
writel(RK_SETBITS(0x50), 0xfdc20140);
diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
index f79f975080..4336d99365 100644
--- a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
@@ -18,6 +18,10 @@ static noinline void rk3568_start(void)
/*
* set iodomain vccio6 to 1.8V needed for GMAC1 to work.
* vccio4 (gmac0/switch) needs to stay at 3v3 (default)
+ * FIXME: This is done by the io-domain driver as well, but there
+ * currently is no mechanism to make sure the driver gets probed
+ * before its consumers. Remove this setup once this issue is
+ * resolved.
*/
//set bit 6 in PMU_GRF_IO_VSEL0 for vccio6 1v8
writel(RK_SETBITS(BIT(6)), PMU_GRF_IO_VSEL0);
diff --git a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
index 363639d21b..9ab436135c 100644
--- a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
@@ -18,6 +18,10 @@ static noinline void rk3568_start(void)
/*
* Enable vccio4 1.8V and vccio6 1.8V
* Needed for GMAC to work.
+ * FIXME: This is done by the io-domain driver as well, but there
+ * currently is no mechanism to make sure the driver gets probed
+ * before its consumers. Remove this setup once this issue is
+ * resolved.
*/
writel(RK_SETBITS(0x50), 0xfdc20140);
--
2.30.2
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-22 8:03 ` Sascha Hauer
@ 2022-09-22 8:13 ` Ahmad Fatoum
2022-09-22 8:23 ` Sascha Hauer
0 siblings, 1 reply; 17+ messages in thread
From: Ahmad Fatoum @ 2022-09-22 8:13 UTC (permalink / raw)
To: Sascha Hauer, Michael Riesch; +Cc: barebox, Frank Wunderlich
Hi,
On 22.09.22 09:03, Sascha Hauer wrote:
> On Wed, Sep 21, 2022 at 01:49:49PM +0200, Michael Riesch wrote:
>> Hi Sascha,
>>
>> On 9/21/22 11:46, Sascha Hauer wrote:
>>> On Mon, Sep 19, 2022 at 01:39:42PM +0200, Michael Riesch wrote:
>>>> The IO domains in Rockchip SoCs need to be configured to match the
>>>> corresponding bank voltage. In Linux this is achieved by means of a
>>>> platform driver that reads the voltage value of the supplies and
>>>> configures the bits in the general register file (GRF) accordingly.
>>>> Port this driver to barebox to provide support for the Rockchip
>>>> RK356x SoCs.
>>>>
>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>>> ---
>>>> drivers/soc/Kconfig | 1 +
>>>> drivers/soc/Makefile | 1 +
>>>> drivers/soc/rockchip/Kconfig | 17 +++
>>>> drivers/soc/rockchip/Makefile | 6 +
>>>> drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
>>>> 5 files changed, 248 insertions(+)
>>>> create mode 100644 drivers/soc/rockchip/Kconfig
>>>> create mode 100644 drivers/soc/rockchip/Makefile
>>>> create mode 100644 drivers/soc/rockchip/io-domain.c
>>>
>>> There's nothing in this driver that makes sure it is probed before the
>>> users of the io domains. What happens when the users are probed before
>>> the io domain driver?
>>
>> Correct. Unfortunately, the situation is quite the same in the kernel.
>> Only recently there was a discussion as to how to resolve this issue
>> [0]. While the RFC in [0] was received well there is no mainline
>> solution to this problem. I would suggest that the mainline kernel
>> solution is adopted once there is one.
>>
>> In the mean time we could
>> - accept that it just works for some reason (TM)
>> - leave the magic bits in the low-level initialization to be on the
>> safe side
>
> Decided for this solution for now.
>
> I added the following as a reminder that there's still something
> to do.
Why not do an of_device_ensure_probed for the I/O domain driver in
rk3568_init()?
>
> Sascha
>
> ---------------------------8<---------------------------
>
> From 502c4e819b80a6ccaefbce4af4c55b830b7e2c73 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Thu, 22 Sep 2022 09:56:57 +0200
> Subject: [PATCH] ARM: Rockchip: Add FIXME comment to io domain setup
>
> We have a io-domain driver for rockchip boards which correctly
> configures the io domain voltages, but currently there is no way
> to make sure the io-domain driver is probed before its consumers.
>
> To be on the safe side keep the io domain setup in the lowlevel code
> for now, but add a comment that it should be removed once this issue
> is resolved.
>
> To solve this issue we need a phandle from the consumers to the io
> domain node. We could add this in barebox locally, but decided to
> wait until the upstream dts files have them.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> arch/arm/boards/radxa-rock3/lowlevel.c | 4 ++++
> arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c | 4 ++++
> arch/arm/boards/rockchip-rk3568-evb/lowlevel.c | 4 ++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/arch/arm/boards/radxa-rock3/lowlevel.c b/arch/arm/boards/radxa-rock3/lowlevel.c
> index 2a449c17ae..a62f60dff8 100644
> --- a/arch/arm/boards/radxa-rock3/lowlevel.c
> +++ b/arch/arm/boards/radxa-rock3/lowlevel.c
> @@ -17,6 +17,10 @@ static noinline void rk3568_start(void)
> /*
> * Enable vccio4 1.8V and vccio6 1.8V
> * Needed for GMAC to work.
> + * FIXME: This is done by the io-domain driver as well, but there
> + * currently is no mechanism to make sure the driver gets probed
> + * before its consumers. Remove this setup once this issue is
> + * resolved.
> */
> writel(RK_SETBITS(0x50), 0xfdc20140);
>
> diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
> index f79f975080..4336d99365 100644
> --- a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
> +++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
> @@ -18,6 +18,10 @@ static noinline void rk3568_start(void)
> /*
> * set iodomain vccio6 to 1.8V needed for GMAC1 to work.
> * vccio4 (gmac0/switch) needs to stay at 3v3 (default)
> + * FIXME: This is done by the io-domain driver as well, but there
> + * currently is no mechanism to make sure the driver gets probed
> + * before its consumers. Remove this setup once this issue is
> + * resolved.
> */
> //set bit 6 in PMU_GRF_IO_VSEL0 for vccio6 1v8
> writel(RK_SETBITS(BIT(6)), PMU_GRF_IO_VSEL0);
> diff --git a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
> index 363639d21b..9ab436135c 100644
> --- a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
> +++ b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
> @@ -18,6 +18,10 @@ static noinline void rk3568_start(void)
> /*
> * Enable vccio4 1.8V and vccio6 1.8V
> * Needed for GMAC to work.
> + * FIXME: This is done by the io-domain driver as well, but there
> + * currently is no mechanism to make sure the driver gets probed
> + * before its consumers. Remove this setup once this issue is
> + * resolved.
> */
> writel(RK_SETBITS(0x50), 0xfdc20140);
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] soc: rockchip: add driver for rockchip io domains
2022-09-22 8:13 ` Ahmad Fatoum
@ 2022-09-22 8:23 ` Sascha Hauer
0 siblings, 0 replies; 17+ messages in thread
From: Sascha Hauer @ 2022-09-22 8:23 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: Michael Riesch, barebox, Frank Wunderlich
On Thu, Sep 22, 2022 at 09:13:35AM +0100, Ahmad Fatoum wrote:
> Hi,
>
> On 22.09.22 09:03, Sascha Hauer wrote:
> > On Wed, Sep 21, 2022 at 01:49:49PM +0200, Michael Riesch wrote:
> >> Hi Sascha,
> >>
> >> On 9/21/22 11:46, Sascha Hauer wrote:
> >>> On Mon, Sep 19, 2022 at 01:39:42PM +0200, Michael Riesch wrote:
> >>>> The IO domains in Rockchip SoCs need to be configured to match the
> >>>> corresponding bank voltage. In Linux this is achieved by means of a
> >>>> platform driver that reads the voltage value of the supplies and
> >>>> configures the bits in the general register file (GRF) accordingly.
> >>>> Port this driver to barebox to provide support for the Rockchip
> >>>> RK356x SoCs.
> >>>>
> >>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >>>> ---
> >>>> drivers/soc/Kconfig | 1 +
> >>>> drivers/soc/Makefile | 1 +
> >>>> drivers/soc/rockchip/Kconfig | 17 +++
> >>>> drivers/soc/rockchip/Makefile | 6 +
> >>>> drivers/soc/rockchip/io-domain.c | 223 +++++++++++++++++++++++++++++++
> >>>> 5 files changed, 248 insertions(+)
> >>>> create mode 100644 drivers/soc/rockchip/Kconfig
> >>>> create mode 100644 drivers/soc/rockchip/Makefile
> >>>> create mode 100644 drivers/soc/rockchip/io-domain.c
> >>>
> >>> There's nothing in this driver that makes sure it is probed before the
> >>> users of the io domains. What happens when the users are probed before
> >>> the io domain driver?
> >>
> >> Correct. Unfortunately, the situation is quite the same in the kernel.
> >> Only recently there was a discussion as to how to resolve this issue
> >> [0]. While the RFC in [0] was received well there is no mainline
> >> solution to this problem. I would suggest that the mainline kernel
> >> solution is adopted once there is one.
> >>
> >> In the mean time we could
> >> - accept that it just works for some reason (TM)
> >> - leave the magic bits in the low-level initialization to be on the
> >> safe side
> >
> > Decided for this solution for now.
> >
> > I added the following as a reminder that there's still something
> > to do.
>
> Why not do an of_device_ensure_probed for the I/O domain driver in
> rk3568_init()?
We can't do a of_device_ensure_probed() properly from plain initcalls.
For example rk3568_init() is called at postcore level, at that time the
io-domain driver is not yet registered.
We would have to find an initcall level where the io-domain driver and
all its dependencies (i2c bus driver, PMIC driver) is registered, but
none of its consumers are probed. While we could probably find the right
initcall level this defeats the purpose of deep probe.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/7] arm: rockchip_v8_defconfig: enable io domain driver
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
2022-09-19 11:39 ` [PATCH v2 1/7] " Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-19 11:39 ` [PATCH v2 3/7] arm: rockchip: radxa-rock3: remove io domain configuration Michael Riesch
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
The IO domain driver is required on RK356x boards to configure
the IO domains correctly. Enable this driver in the defconfig.
Since the corresponding voltages are usually supplied by the
RK80x companion PMIC, enable the RK808 driver as well.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
arch/arm/configs/rockchip_v8_defconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/configs/rockchip_v8_defconfig b/arch/arm/configs/rockchip_v8_defconfig
index 7fba31ddc3..d673f7f788 100644
--- a/arch/arm/configs/rockchip_v8_defconfig
+++ b/arch/arm/configs/rockchip_v8_defconfig
@@ -139,3 +139,7 @@ CONFIG_FS_FAT_LFN=y
CONFIG_FS_BPKFS=y
CONFIG_FS_UIMAGEFS=y
CONFIG_LZO_DECOMPRESS=y
+CONFIG_I2C_RK3X=y
+CONFIG_MFD_RK808=y
+CONFIG_REGULATOR_RK808=y
+CONFIG_ROCKCHIP_IODOMAIN=y
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/7] arm: rockchip: radxa-rock3: remove io domain configuration
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
2022-09-19 11:39 ` [PATCH v2 1/7] " Michael Riesch
2022-09-19 11:39 ` [PATCH v2 2/7] arm: rockchip_v8_defconfig: enable io domain driver Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-19 11:39 ` [PATCH v2 4/7] arm: rockchip: rk3568-evb: " Michael Riesch
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
Let the recently added Rockchip IO domain driver configure the IO
domains instead of setting the bits in rk3568_start().
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
arch/arm/boards/radxa-rock3/lowlevel.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/arm/boards/radxa-rock3/lowlevel.c b/arch/arm/boards/radxa-rock3/lowlevel.c
index 00a68889cd..5c2499362b 100644
--- a/arch/arm/boards/radxa-rock3/lowlevel.c
+++ b/arch/arm/boards/radxa-rock3/lowlevel.c
@@ -23,12 +23,6 @@ static noinline void rk3568_start(void *fdt)
setup_c();
- /*
- * Enable vccio4 1.8V and vccio6 1.8V
- * Needed for GMAC to work.
- */
- writel(RK_SETBITS(0x50), 0xfdc20140);
-
if (current_el() == 3) {
rk3568_lowlevel_init();
rk3568_atf_load_bl31(fdt);
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/7] arm: rockchip: rk3568-evb: remove io domain configuration
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
` (2 preceding siblings ...)
2022-09-19 11:39 ` [PATCH v2 3/7] arm: rockchip: radxa-rock3: remove io domain configuration Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-19 11:39 ` [PATCH v2 5/7] arm: rockchip: rk3568-bpi-r2pro: " Michael Riesch
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
Let the recently added Rockchip IO domain driver configure the IO
domains instead of setting the bits in rk3568_start().
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
arch/arm/boards/rockchip-rk3568-evb/lowlevel.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
index 363639d21b..9c1c7be8a6 100644
--- a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
@@ -15,12 +15,6 @@ static noinline void rk3568_start(void)
{
void *fdt;
- /*
- * Enable vccio4 1.8V and vccio6 1.8V
- * Needed for GMAC to work.
- */
- writel(RK_SETBITS(0x50), 0xfdc20140);
-
fdt = __dtb_rk3568_evb1_v10_start;
if (current_el() == 3) {
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 5/7] arm: rockchip: rk3568-bpi-r2pro: remove io domain configuration
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
` (3 preceding siblings ...)
2022-09-19 11:39 ` [PATCH v2 4/7] arm: rockchip: rk3568-evb: " Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-19 11:39 ` [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method Michael Riesch
2022-09-19 11:39 ` [PATCH v2 7/7] arm: rockchip: rk3568-bpi-r2pro: use common method rk3568_start Michael Riesch
6 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
Let the recently added Rockchip IO domain driver configure the IO
domains instead of setting the bits in rk3568_start().
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
index f79f975080..36f7acd7d0 100644
--- a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
@@ -15,15 +15,6 @@ static noinline void rk3568_start(void)
{
void *fdt;
- /*
- * set iodomain vccio6 to 1.8V needed for GMAC1 to work.
- * vccio4 (gmac0/switch) needs to stay at 3v3 (default)
- */
- //set bit 6 in PMU_GRF_IO_VSEL0 for vccio6 1v8
- writel(RK_SETBITS(BIT(6)), PMU_GRF_IO_VSEL0);
- //clear bit 6 for 3v3 as it was set to 1v8
- writel(RK_CLRBITS(BIT(6)), PMU_GRF_IO_VSEL1);
-
fdt = __dtb_rk3568_bpi_r2_pro_start;
if (current_el() == 3) {
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
` (4 preceding siblings ...)
2022-09-19 11:39 ` [PATCH v2 5/7] arm: rockchip: rk3568-bpi-r2pro: " Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
2022-09-21 9:17 ` Sascha Hauer
2022-09-19 11:39 ` [PATCH v2 7/7] arm: rockchip: rk3568-bpi-r2pro: use common method rk3568_start Michael Riesch
6 siblings, 1 reply; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
After the removal of the IO domain configuration code, the low-level
initialization is the same for all RK356x boards. Add a common
method rk3568_start to remove this code duplication.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
arch/arm/boards/pine64-quartz64/lowlevel.c | 30 +----------------
arch/arm/boards/radxa-rock3/lowlevel.c | 27 ---------------
.../arm/boards/rockchip-rk3568-evb/lowlevel.c | 33 +------------------
.../arm/mach-rockchip/include/mach/rockchip.h | 1 +
arch/arm/mach-rockchip/rk3568.c | 24 ++++++++++++++
5 files changed, 27 insertions(+), 88 deletions(-)
diff --git a/arch/arm/boards/pine64-quartz64/lowlevel.c b/arch/arm/boards/pine64-quartz64/lowlevel.c
index b295885522..e1beb3e624 100644
--- a/arch/arm/boards/pine64-quartz64/lowlevel.c
+++ b/arch/arm/boards/pine64-quartz64/lowlevel.c
@@ -1,39 +1,11 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <common.h>
-#include <linux/sizes.h>
-#include <asm/barebox-arm-head.h>
#include <asm/barebox-arm.h>
-#include <mach/hardware.h>
-#include <mach/atf.h>
-#include <debug_ll.h>
#include <mach/rockchip.h>
extern char __dtb_rk3566_quartz64_a_start[];
-static noinline void start_quartz64(void *fdt)
-{
- /*
- * Image execution starts at 0x0, but this is used for ATF and
- * OP-TEE later, so move away from here.
- */
- if (current_el() == 3)
- relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
- else
- relocate_to_current_adr();
-
- setup_c();
-
- if (current_el() == 3) {
- rk3568_lowlevel_init();
- rk3568_atf_load_bl31(fdt);
- /* not reached */
- }
-
- barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM,
- fdt);
-}
-
ENTRY_FUNCTION(start_quartz64a, r0, r1, r2)
{
- start_quartz64(__dtb_rk3566_quartz64_a_start);
+ rk3568_start(__dtb_rk3566_quartz64_a_start);
}
diff --git a/arch/arm/boards/radxa-rock3/lowlevel.c b/arch/arm/boards/radxa-rock3/lowlevel.c
index 5c2499362b..9875a3966b 100644
--- a/arch/arm/boards/radxa-rock3/lowlevel.c
+++ b/arch/arm/boards/radxa-rock3/lowlevel.c
@@ -1,37 +1,10 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <common.h>
-#include <linux/sizes.h>
-#include <asm/barebox-arm-head.h>
#include <asm/barebox-arm.h>
-#include <mach/hardware.h>
-#include <mach/atf.h>
-#include <debug_ll.h>
#include <mach/rockchip.h>
extern char __dtb_rk3568_rock_3a_start[];
-static noinline void rk3568_start(void *fdt)
-{
- /*
- * Image execution starts at 0x0, but this is used for ATF and
- * OP-TEE later, so move away from here.
- */
- if (current_el() == 3)
- relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
- else
- relocate_to_current_adr();
-
- setup_c();
-
- if (current_el() == 3) {
- rk3568_lowlevel_init();
- rk3568_atf_load_bl31(fdt);
- /* not reached */
- }
-
- barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM, fdt);
-}
-
ENTRY_FUNCTION(start_rock3a, r0, r1, r2)
{
rk3568_start(__dtb_rk3568_rock_3a_start);
diff --git a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
index 9c1c7be8a6..4954e9a3cc 100644
--- a/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-evb/lowlevel.c
@@ -1,43 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <common.h>
-#include <linux/sizes.h>
-#include <asm/barebox-arm-head.h>
#include <asm/barebox-arm.h>
-#include <mach/hardware.h>
-#include <mach/atf.h>
-#include <debug_ll.h>
#include <mach/rockchip.h>
extern char __dtb_rk3568_evb1_v10_start[];
-static noinline void rk3568_start(void)
-{
- void *fdt;
-
- fdt = __dtb_rk3568_evb1_v10_start;
-
- if (current_el() == 3) {
- rk3568_lowlevel_init();
- rk3568_atf_load_bl31(fdt);
- /* not reached */
- }
-
- barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM, fdt);
-}
-
ENTRY_FUNCTION(start_rk3568_evb, r0, r1, r2)
{
- /*
- * Image execution starts at 0x0, but this is used for ATF and
- * OP-TEE later, so move away from here.
- */
- if (current_el() == 3)
- relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
- else
- relocate_to_current_adr();
-
- setup_c();
-
- rk3568_start();
+ rk3568_start(__dtb_rk3568_evb1_v10_start);
}
diff --git a/arch/arm/mach-rockchip/include/mach/rockchip.h b/arch/arm/mach-rockchip/include/mach/rockchip.h
index ff8b1109f8..7b681c6ce3 100644
--- a/arch/arm/mach-rockchip/include/mach/rockchip.h
+++ b/arch/arm/mach-rockchip/include/mach/rockchip.h
@@ -34,5 +34,6 @@ static inline int rk3568_init(void)
#endif
void rk3568_lowlevel_init(void);
+void rk3568_start(void *fdt);
#endif /* __MACH_ROCKCHIP_H */
diff --git a/arch/arm/mach-rockchip/rk3568.c b/arch/arm/mach-rockchip/rk3568.c
index 19dfa9b871..26bc4a06eb 100644
--- a/arch/arm/mach-rockchip/rk3568.c
+++ b/arch/arm/mach-rockchip/rk3568.c
@@ -2,8 +2,10 @@
#include <common.h>
#include <io.h>
#include <bootsource.h>
+#include <mach/atf.h>
#include <mach/rk3568-regs.h>
#include <mach/rockchip.h>
+#include <asm/barebox-arm.h>
#define GRF_BASE 0xfdc60000
#define GRF_GPIO1B_DS_2 0x218
@@ -169,3 +171,25 @@ int rk3568_init(void)
return 0;
}
+
+void rk3568_start(void *fdt)
+{
+ /*
+ * Image execution starts at 0x0, but this is used for ATF and
+ * OP-TEE later, so move away from here.
+ */
+ if (current_el() == 3)
+ relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
+ else
+ relocate_to_current_adr();
+
+ setup_c();
+
+ if (current_el() == 3) {
+ rk3568_lowlevel_init();
+ rk3568_atf_load_bl31(fdt);
+ /* not reached */
+ }
+
+ barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM, fdt);
+}
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method
2022-09-19 11:39 ` [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method Michael Riesch
@ 2022-09-21 9:17 ` Sascha Hauer
2022-09-21 11:57 ` Michael Riesch
0 siblings, 1 reply; 17+ messages in thread
From: Sascha Hauer @ 2022-09-21 9:17 UTC (permalink / raw)
To: Michael Riesch; +Cc: barebox, Frank Wunderlich
On Mon, Sep 19, 2022 at 01:39:47PM +0200, Michael Riesch wrote:
> After the removal of the IO domain configuration code, the low-level
> initialization is the same for all RK356x boards. Add a common
> method rk3568_start to remove this code duplication.
>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> arch/arm/boards/pine64-quartz64/lowlevel.c | 30 +----------------
> arch/arm/boards/radxa-rock3/lowlevel.c | 27 ---------------
> .../arm/boards/rockchip-rk3568-evb/lowlevel.c | 33 +------------------
> .../arm/mach-rockchip/include/mach/rockchip.h | 1 +
> arch/arm/mach-rockchip/rk3568.c | 24 ++++++++++++++
> 5 files changed, 27 insertions(+), 88 deletions(-)
>
> diff --git a/arch/arm/boards/pine64-quartz64/lowlevel.c b/arch/arm/boards/pine64-quartz64/lowlevel.c
> index b295885522..e1beb3e624 100644
> --- a/arch/arm/boards/pine64-quartz64/lowlevel.c
> +++ b/arch/arm/boards/pine64-quartz64/lowlevel.c
> @@ -1,39 +1,11 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include <common.h>
> -#include <linux/sizes.h>
> -#include <asm/barebox-arm-head.h>
> #include <asm/barebox-arm.h>
> -#include <mach/hardware.h>
> -#include <mach/atf.h>
> -#include <debug_ll.h>
> #include <mach/rockchip.h>
>
> extern char __dtb_rk3566_quartz64_a_start[];
>
> -static noinline void start_quartz64(void *fdt)
> -{
> - /*
> - * Image execution starts at 0x0, but this is used for ATF and
> - * OP-TEE later, so move away from here.
> - */
> - if (current_el() == 3)
> - relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
> - else
> - relocate_to_current_adr();
> -
> - setup_c();
> -
> - if (current_el() == 3) {
> - rk3568_lowlevel_init();
> - rk3568_atf_load_bl31(fdt);
> - /* not reached */
> - }
> -
> - barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM,
> - fdt);
> -}
> -
> ENTRY_FUNCTION(start_quartz64a, r0, r1, r2)
> {
> - start_quartz64(__dtb_rk3566_quartz64_a_start);
> + rk3568_start(__dtb_rk3566_quartz64_a_start);
> }
Here __dtb_rk3566_quartz64_a_start is accessed before setup_c() has been
called. That is not allowed, see the patch I just sent.
BTW this patch breaks compilation, which is only fixed in 7/7:
/ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c:14:22: error: conflicting types for 'rk3568_start'
14 | static noinline void rk3568_start(void)
| ^~~~~~~~~~~~
In file included from /ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c:10:
/ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/mach-rockchip/include/mach/rockchip.h:37:6: note: previous declaration of 'rk3568_start' was here
37 | void rk3568_start(void *fdt);
| ^~~~~~~~~~~~
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method
2022-09-21 9:17 ` Sascha Hauer
@ 2022-09-21 11:57 ` Michael Riesch
2022-09-21 12:21 ` Sascha Hauer
0 siblings, 1 reply; 17+ messages in thread
From: Michael Riesch @ 2022-09-21 11:57 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox, Frank Wunderlich
Hi Sascha,
On 9/21/22 11:17, Sascha Hauer wrote:
> On Mon, Sep 19, 2022 at 01:39:47PM +0200, Michael Riesch wrote:
>> After the removal of the IO domain configuration code, the low-level
>> initialization is the same for all RK356x boards. Add a common
>> method rk3568_start to remove this code duplication.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>> arch/arm/boards/pine64-quartz64/lowlevel.c | 30 +----------------
>> arch/arm/boards/radxa-rock3/lowlevel.c | 27 ---------------
>> .../arm/boards/rockchip-rk3568-evb/lowlevel.c | 33 +------------------
>> .../arm/mach-rockchip/include/mach/rockchip.h | 1 +
>> arch/arm/mach-rockchip/rk3568.c | 24 ++++++++++++++
>> 5 files changed, 27 insertions(+), 88 deletions(-)
>>
>> diff --git a/arch/arm/boards/pine64-quartz64/lowlevel.c b/arch/arm/boards/pine64-quartz64/lowlevel.c
>> index b295885522..e1beb3e624 100644
>> --- a/arch/arm/boards/pine64-quartz64/lowlevel.c
>> +++ b/arch/arm/boards/pine64-quartz64/lowlevel.c
>> @@ -1,39 +1,11 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> #include <common.h>
>> -#include <linux/sizes.h>
>> -#include <asm/barebox-arm-head.h>
>> #include <asm/barebox-arm.h>
>> -#include <mach/hardware.h>
>> -#include <mach/atf.h>
>> -#include <debug_ll.h>
>> #include <mach/rockchip.h>
>>
>> extern char __dtb_rk3566_quartz64_a_start[];
>>
>> -static noinline void start_quartz64(void *fdt)
>> -{
>> - /*
>> - * Image execution starts at 0x0, but this is used for ATF and
>> - * OP-TEE later, so move away from here.
>> - */
>> - if (current_el() == 3)
>> - relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
>> - else
>> - relocate_to_current_adr();
>> -
>> - setup_c();
>> -
>> - if (current_el() == 3) {
>> - rk3568_lowlevel_init();
>> - rk3568_atf_load_bl31(fdt);
>> - /* not reached */
>> - }
>> -
>> - barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM,
>> - fdt);
>> -}
>> -
>> ENTRY_FUNCTION(start_quartz64a, r0, r1, r2)
>> {
>> - start_quartz64(__dtb_rk3566_quartz64_a_start);
>> + rk3568_start(__dtb_rk3566_quartz64_a_start);
>> }
>
> Here __dtb_rk3566_quartz64_a_start is accessed before setup_c() has been
> called. That is not allowed, see the patch I just sent.
Does the refactoring make sense to you in general? Can I change it to
ENTRY_FUNCTION(start_my_fancy_board, r0, r1, r2)
{
setup_c();
rk3568_start(__dtb_my_fancy_board_start);
}
?
> BTW this patch breaks compilation, which is only fixed in 7/7:
>
> /ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c:14:22: error: conflicting types for 'rk3568_start'
> 14 | static noinline void rk3568_start(void)
> | ^~~~~~~~~~~~
> In file included from /ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c:10:
> /ptx/work/WORK_EIHEI/sha/projects/barebox/barebox/arch/arm/mach-rockchip/include/mach/rockchip.h:37:6: note: previous declaration of 'rk3568_start' was here
> 37 | void rk3568_start(void *fdt);
> | ^~~~~~~~~~~~
Oops. The goal was to separate the changes that affect the BananaPi, but
in this case 6 + 7 need to be squashed. I guess Frank needs to object to
the combined patch if it breaks something on his side.
Best regards,
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method
2022-09-21 11:57 ` Michael Riesch
@ 2022-09-21 12:21 ` Sascha Hauer
2022-09-21 12:34 ` Michael Riesch
0 siblings, 1 reply; 17+ messages in thread
From: Sascha Hauer @ 2022-09-21 12:21 UTC (permalink / raw)
To: Michael Riesch; +Cc: barebox, Frank Wunderlich
On Wed, Sep 21, 2022 at 01:57:25PM +0200, Michael Riesch wrote:
> Hi Sascha,
>
> On 9/21/22 11:17, Sascha Hauer wrote:
> > On Mon, Sep 19, 2022 at 01:39:47PM +0200, Michael Riesch wrote:
> >> After the removal of the IO domain configuration code, the low-level
> >> initialization is the same for all RK356x boards. Add a common
> >> method rk3568_start to remove this code duplication.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >> ---
> >> arch/arm/boards/pine64-quartz64/lowlevel.c | 30 +----------------
> >> arch/arm/boards/radxa-rock3/lowlevel.c | 27 ---------------
> >> .../arm/boards/rockchip-rk3568-evb/lowlevel.c | 33 +------------------
> >> .../arm/mach-rockchip/include/mach/rockchip.h | 1 +
> >> arch/arm/mach-rockchip/rk3568.c | 24 ++++++++++++++
> >> 5 files changed, 27 insertions(+), 88 deletions(-)
> >>
> >> diff --git a/arch/arm/boards/pine64-quartz64/lowlevel.c b/arch/arm/boards/pine64-quartz64/lowlevel.c
> >> index b295885522..e1beb3e624 100644
> >> --- a/arch/arm/boards/pine64-quartz64/lowlevel.c
> >> +++ b/arch/arm/boards/pine64-quartz64/lowlevel.c
> >> @@ -1,39 +1,11 @@
> >> // SPDX-License-Identifier: GPL-2.0-only
> >> #include <common.h>
> >> -#include <linux/sizes.h>
> >> -#include <asm/barebox-arm-head.h>
> >> #include <asm/barebox-arm.h>
> >> -#include <mach/hardware.h>
> >> -#include <mach/atf.h>
> >> -#include <debug_ll.h>
> >> #include <mach/rockchip.h>
> >>
> >> extern char __dtb_rk3566_quartz64_a_start[];
> >>
> >> -static noinline void start_quartz64(void *fdt)
> >> -{
> >> - /*
> >> - * Image execution starts at 0x0, but this is used for ATF and
> >> - * OP-TEE later, so move away from here.
> >> - */
> >> - if (current_el() == 3)
> >> - relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
> >> - else
> >> - relocate_to_current_adr();
> >> -
> >> - setup_c();
> >> -
> >> - if (current_el() == 3) {
> >> - rk3568_lowlevel_init();
> >> - rk3568_atf_load_bl31(fdt);
> >> - /* not reached */
> >> - }
> >> -
> >> - barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM,
> >> - fdt);
> >> -}
> >> -
> >> ENTRY_FUNCTION(start_quartz64a, r0, r1, r2)
> >> {
> >> - start_quartz64(__dtb_rk3566_quartz64_a_start);
> >> + rk3568_start(__dtb_rk3566_quartz64_a_start);
> >> }
> >
> > Here __dtb_rk3566_quartz64_a_start is accessed before setup_c() has been
> > called. That is not allowed, see the patch I just sent.
>
> Does the refactoring make sense to you in general? Can I change it to
>
> ENTRY_FUNCTION(start_my_fancy_board, r0, r1, r2)
> {
> setup_c();
> rk3568_start(__dtb_my_fancy_board_start);
> }
Well it's not only setup_c() but also the relocate_to_adr_full() or
relocate_to_current_adr() part that has to be called before setup_c().
At that point there is not much left to factor out to a common function.
Unless you want to turn this into preprocessor macros (and I don't
recommand doing that) my suggestion is that we just live with this bit
of code duplication.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method
2022-09-21 12:21 ` Sascha Hauer
@ 2022-09-21 12:34 ` Michael Riesch
0 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-21 12:34 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox, Frank Wunderlich
Hi Sascha,
On 9/21/22 14:21, Sascha Hauer wrote:
> On Wed, Sep 21, 2022 at 01:57:25PM +0200, Michael Riesch wrote:
>> Hi Sascha,
>>
>> On 9/21/22 11:17, Sascha Hauer wrote:
>>> On Mon, Sep 19, 2022 at 01:39:47PM +0200, Michael Riesch wrote:
>>>> After the removal of the IO domain configuration code, the low-level
>>>> initialization is the same for all RK356x boards. Add a common
>>>> method rk3568_start to remove this code duplication.
>>>>
>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>>> ---
>>>> arch/arm/boards/pine64-quartz64/lowlevel.c | 30 +----------------
>>>> arch/arm/boards/radxa-rock3/lowlevel.c | 27 ---------------
>>>> .../arm/boards/rockchip-rk3568-evb/lowlevel.c | 33 +------------------
>>>> .../arm/mach-rockchip/include/mach/rockchip.h | 1 +
>>>> arch/arm/mach-rockchip/rk3568.c | 24 ++++++++++++++
>>>> 5 files changed, 27 insertions(+), 88 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boards/pine64-quartz64/lowlevel.c b/arch/arm/boards/pine64-quartz64/lowlevel.c
>>>> index b295885522..e1beb3e624 100644
>>>> --- a/arch/arm/boards/pine64-quartz64/lowlevel.c
>>>> +++ b/arch/arm/boards/pine64-quartz64/lowlevel.c
>>>> @@ -1,39 +1,11 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> #include <common.h>
>>>> -#include <linux/sizes.h>
>>>> -#include <asm/barebox-arm-head.h>
>>>> #include <asm/barebox-arm.h>
>>>> -#include <mach/hardware.h>
>>>> -#include <mach/atf.h>
>>>> -#include <debug_ll.h>
>>>> #include <mach/rockchip.h>
>>>>
>>>> extern char __dtb_rk3566_quartz64_a_start[];
>>>>
>>>> -static noinline void start_quartz64(void *fdt)
>>>> -{
>>>> - /*
>>>> - * Image execution starts at 0x0, but this is used for ATF and
>>>> - * OP-TEE later, so move away from here.
>>>> - */
>>>> - if (current_el() == 3)
>>>> - relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
>>>> - else
>>>> - relocate_to_current_adr();
>>>> -
>>>> - setup_c();
>>>> -
>>>> - if (current_el() == 3) {
>>>> - rk3568_lowlevel_init();
>>>> - rk3568_atf_load_bl31(fdt);
>>>> - /* not reached */
>>>> - }
>>>> -
>>>> - barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM,
>>>> - fdt);
>>>> -}
>>>> -
>>>> ENTRY_FUNCTION(start_quartz64a, r0, r1, r2)
>>>> {
>>>> - start_quartz64(__dtb_rk3566_quartz64_a_start);
>>>> + rk3568_start(__dtb_rk3566_quartz64_a_start);
>>>> }
>>>
>>> Here __dtb_rk3566_quartz64_a_start is accessed before setup_c() has been
>>> called. That is not allowed, see the patch I just sent.
>>
>> Does the refactoring make sense to you in general? Can I change it to
>>
>> ENTRY_FUNCTION(start_my_fancy_board, r0, r1, r2)
>> {
>> setup_c();
>> rk3568_start(__dtb_my_fancy_board_start);
>> }
>
> Well it's not only setup_c() but also the relocate_to_adr_full() or
> relocate_to_current_adr() part that has to be called before setup_c().
Ah OK! Thanks for the clarification.
> At that point there is not much left to factor out to a common function.
>
> Unless you want to turn this into preprocessor macros (and I don't
> recommand doing that) my suggestion is that we just live with this bit
> of code duplication.
OK, patches 6+7 can be dropped, then.
Best regards,
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 7/7] arm: rockchip: rk3568-bpi-r2pro: use common method rk3568_start
2022-09-19 11:39 [PATCH v2 0/7] soc: rockchip: add driver for rockchip io domains Michael Riesch
` (5 preceding siblings ...)
2022-09-19 11:39 ` [PATCH v2 6/7] arm: rockchip: rk3568: refactor common rk3568_start method Michael Riesch
@ 2022-09-19 11:39 ` Michael Riesch
6 siblings, 0 replies; 17+ messages in thread
From: Michael Riesch @ 2022-09-19 11:39 UTC (permalink / raw)
To: barebox; +Cc: Frank Wunderlich, Michael Riesch
After the removal of the IO domain configuration code, the common
low-level initialization method rk3568_start can be used to remove
code duplication.
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
.../rockchip-rk3568-bpi-r2pro/lowlevel.c | 33 +------------------
1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
index 36f7acd7d0..8b04dccc1e 100644
--- a/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
+++ b/arch/arm/boards/rockchip-rk3568-bpi-r2pro/lowlevel.c
@@ -1,43 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <common.h>
-#include <linux/sizes.h>
-#include <asm/barebox-arm-head.h>
#include <asm/barebox-arm.h>
-#include <mach/hardware.h>
-#include <mach/atf.h>
-#include <debug_ll.h>
#include <mach/rockchip.h>
extern char __dtb_rk3568_bpi_r2_pro_start[];
-static noinline void rk3568_start(void)
-{
- void *fdt;
-
- fdt = __dtb_rk3568_bpi_r2_pro_start;
-
- if (current_el() == 3) {
- rk3568_lowlevel_init();
- rk3568_atf_load_bl31(fdt);
- /* not reached */
- }
-
- barebox_arm_entry(RK3568_DRAM_BOTTOM, 0x80000000 - RK3568_DRAM_BOTTOM, fdt);
-}
-
ENTRY_FUNCTION(start_rk3568_bpi_r2pro, r0, r1, r2)
{
- /*
- * Image execution starts at 0x0, but this is used for ATF and
- * OP-TEE later, so move away from here.
- */
- if (current_el() == 3)
- relocate_to_adr_full(RK3568_BAREBOX_LOAD_ADDRESS);
- else
- relocate_to_current_adr();
-
- setup_c();
-
- rk3568_start();
+ rk3568_start(__dtb_rk3568_bpi_r2_pro_start);
}
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread