[PATCH v2 0/2] ARM: Add Rockchip rk3288w support

[PATCH v2 0/2] ARM: Add Rockchip rk3288w support

[Mylène Josserand]

Hello everyone,

Here is my V2 of my patches that add the support for the Rockchip
RK3288w which is a revision of the RK3288. It is mostly the same SOC
except for, at least, one clock tree which is different.
This difference is only known by looking at the BSP kernel [1].

Currently, the mainline kernel will not hang on rk3288w but it is
probably by "chance" because we got an issue on a lower kernel version.

According to Rockchip's U-Boot [2], the rk3288w can be detected using
the HDMI revision number (= 0x1A) in this version of the SOC.
Not to rely on U-Boot about the compatible, the patch 01 will handle
the detection of the HDMI version.

In this V2, the revision's detection is done using soc_device
registration. Thanks to that, in case of other differences than the clock
tree, it will be possible to detect rk3288/rk3288w using the 'soc_device_match'
function.
The main issue was an ordering issue: my rk3288 driver was
registered too late to be able to act on the clock tree. This is fixed
by using an initcall in the clock driver. One possible way would be to
convert the clock driver into platform_driver but, as it is using
some common functions to all Rockchip's drivers, it would have been
necessary to update all others. Instead, using an initcall to post-pone
hclkvio clock's registration is enough to make everything work correctly
without a big change on the clock driver.

Changes since v1:
   - As suggested by Geert, update the HDMI detection by using all
   'soc_device' functions
   - Use 'soc_device_match' function to detect the revision in the
   clock driver
   - Create a function that registers hclkvio clocks later than others
   to be sure that RK3288 revision is read.

Best regards,
Mylène Josserand

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/clk/rockchip/clk-rk3288.c#L960..L964
[2] https://github.com/rockchip-linux/u-boot/blob/next-dev/arch/arm/mach-rockchip/rk3288/rk3288.c#L378..L388

Mylène Josserand (2):
  soc: rockchip: Register a soc_device to retrieve revision
  clk: rockchip: rk3288: Handle clock tree for rk3288w

 drivers/clk/rockchip/clk-rk3288.c |  36 ++++++++-
 drivers/soc/rockchip/Makefile     |   1 +
 drivers/soc/rockchip/rk3288.c     | 125 ++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+), 4 deletions(-)
 create mode 100644 drivers/soc/rockchip/rk3288.c

-- 
2.25.1

[PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision

[Mylène Josserand]

Determine which revision of rk3288 by checking the HDMI version.
According to the Rockchip BSP kernel/u-boot, on rk3288w, the HDMI
revision equals 0x1A which is not the case for the rk3288.

As these SOC have some differences, this driver will help us
to know on which revision we are by using 'soc_device' registration
to be able to use 'soc_device_match' to detect rk3288/rk3288w.

Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
---
 drivers/soc/rockchip/Makefile |   1 +
 drivers/soc/rockchip/rk3288.c | 125 ++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 drivers/soc/rockchip/rk3288.c

diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
index afca0a4c4b72..9dbf12913512 100644
--- a/drivers/soc/rockchip/Makefile
+++ b/drivers/soc/rockchip/Makefile
@@ -2,5 +2,6 @@
 #
 # Rockchip Soc drivers
 #
+obj-$(CONFIG_ARCH_ROCKCHIP) += rk3288.o
 obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
 obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
diff --git a/drivers/soc/rockchip/rk3288.c b/drivers/soc/rockchip/rk3288.c
new file mode 100644
index 000000000000..83379ba2b31b
--- /dev/null
+++ b/drivers/soc/rockchip/rk3288.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Collabora Ltd
+ * Author: Mylene Josserand <mylene.josserand@collabora.com>
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/sys_soc.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#define RK3288_HDMI_REV_REG	0x04
+#define RK3288W_HDMI_REV	0x1A
+
+enum rk3288_soc_rev {
+	RK3288_SOC_REV_NOT_DETECT,
+	RK3288_SOC_REV_RK3288,
+	RK3288_SOC_REV_RK3288W,
+};
+
+static int rk3288_revision(void)
+{
+	static int revision = RK3288_SOC_REV_NOT_DETECT;
+	struct device_node *dn;
+	void __iomem *hdmi_base;
+
+	if (revision != RK3288_SOC_REV_NOT_DETECT)
+		return revision;
+
+	dn = of_find_compatible_node(NULL, NULL, "rockchip,rk3288-dw-hdmi");
+	if (!dn) {
+		pr_err("%s: Couldn't find HDMI node\n", __func__);
+		return -EINVAL;
+	}
+
+	hdmi_base = of_iomap(dn, 0);
+	of_node_put(dn);
+
+	if (!hdmi_base) {
+		pr_err("%s: Couldn't map %pOF regs\n", __func__,
+		       hdmi_base);
+		return -ENXIO;
+	}
+
+	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG)
+	    == RK3288W_HDMI_REV)
+		revision = RK3288_SOC_REV_RK3288W;
+	else
+		revision = RK3288_SOC_REV_RK3288;
+
+	iounmap(hdmi_base);
+
+	return revision;
+}
+
+static const char *rk3288_socinfo_revision(u32 rev)
+{
+	const char *soc_rev;
+
+	switch (rev) {
+	case RK3288_SOC_REV_RK3288:
+		soc_rev = "RK3288";
+		break;
+
+	case RK3288_SOC_REV_RK3288W:
+		soc_rev = "RK3288w";
+		break;
+
+	case RK3288_SOC_REV_NOT_DETECT:
+		soc_rev = "";
+		break;
+
+	default:
+		soc_rev = "unknown";
+		break;
+	}
+
+	return kstrdup_const(soc_rev, GFP_KERNEL);
+}
+
+static const struct of_device_id rk3288_soc_match[] = {
+	{ .compatible = "rockchip,rk3288", },
+	{ }
+};
+
+static int __init rk3288_soc_init(void)
+{
+	struct soc_device_attribute *soc_dev_attr;
+	struct soc_device *soc_dev;
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, rk3288_soc_match);
+	if (!np)
+		return -ENODEV;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	soc_dev_attr->family = "Rockchip";
+	soc_dev_attr->soc_id = "RK32xx";
+
+	np = of_find_node_by_path("/");
+	of_property_read_string(np, "model", &soc_dev_attr->machine);
+	of_node_put(np);
+
+	soc_dev_attr->revision = rk3288_socinfo_revision(rk3288_revision());
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree_const(soc_dev_attr->revision);
+		kfree_const(soc_dev_attr->soc_id);
+		kfree(soc_dev_attr);
+		return PTR_ERR(soc_dev);
+	}
+
+	dev_info(soc_device_to_device(soc_dev), "Rockchip %s %s detected\n",
+		 soc_dev_attr->soc_id, soc_dev_attr->revision);
+
+	return 0;
+}
+postcore_initcall(rk3288_soc_init);
-- 
2.25.1

Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision

[Heiko Stübner]

Hi Mylène,

Am Mittwoch, 1. April 2020, 17:35:12 CEST schrieb Mylène Josserand:
> Determine which revision of rk3288 by checking the HDMI version.
> According to the Rockchip BSP kernel/u-boot, on rk3288w, the HDMI
> revision equals 0x1A which is not the case for the rk3288.
> 
> As these SOC have some differences, this driver will help us
> to know on which revision we are by using 'soc_device' registration
> to be able to use 'soc_device_match' to detect rk3288/rk3288w.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>

I like your new approach quite a lot :-)

There are some things we need to take into account though, see below.


> ---
>  drivers/soc/rockchip/Makefile |   1 +
>  drivers/soc/rockchip/rk3288.c | 125 ++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 drivers/soc/rockchip/rk3288.c
> 
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index afca0a4c4b72..9dbf12913512 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -2,5 +2,6 @@
>  #
>  # Rockchip Soc drivers
>  #
> +obj-$(CONFIG_ARCH_ROCKCHIP) += rk3288.o
>  obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> diff --git a/drivers/soc/rockchip/rk3288.c b/drivers/soc/rockchip/rk3288.c

I'd really like this to be a soc.c instead of rk3288.c ;-)


> new file mode 100644
> index 000000000000..83379ba2b31b
> --- /dev/null
> +++ b/drivers/soc/rockchip/rk3288.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Collabora Ltd
> + * Author: Mylene Josserand <mylene.josserand@collabora.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#define RK3288_HDMI_REV_REG	0x04
> +#define RK3288W_HDMI_REV	0x1A
> +
> +enum rk3288_soc_rev {
> +	RK3288_SOC_REV_NOT_DETECT,
> +	RK3288_SOC_REV_RK3288,
> +	RK3288_SOC_REV_RK3288W,
> +};
> +
> +static int rk3288_revision(void)
> +{
> +	static int revision = RK3288_SOC_REV_NOT_DETECT;
> +	struct device_node *dn;
> +	void __iomem *hdmi_base;
> +
> +	if (revision != RK3288_SOC_REV_NOT_DETECT)
> +		return revision;
> +
> +	dn = of_find_compatible_node(NULL, NULL, "rockchip,rk3288-dw-hdmi");
> +	if (!dn) {
> +		pr_err("%s: Couldn't find HDMI node\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	hdmi_base = of_iomap(dn, 0);
> +	of_node_put(dn);
> +
> +	if (!hdmi_base) {
> +		pr_err("%s: Couldn't map %pOF regs\n", __func__,
> +		       hdmi_base);
> +		return -ENXIO;
> +	}

The possible problem I see here is clocking and power-domain of the hdmi
controller in corner-cases. In the past we already had a lot of fun with
kexec, which also indicates that people actually use kexec productively.

So while all clocks are ungated and all power-domains are powered on first
boot, on a system without graphics the pclk+power-domain could be off when
doing a kexec into a second kernel, which then would probably hang here.


Of course with the hdmi-pclk being sourced from hclk_vio we run into a
chicken-egg-problem, as we need pclk_hdmi_ctrl to register hclk_vio at all.

So I guess one way out of this could be to
- amend rk3288_clk_shutdown() to also ungate the hdmi-pclk on shutdown
- add a shutdown mechanism to the power-domain driver so that it can
  enable PD_VIO on shutdown

> +
> +	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG)
> +	    == RK3288W_HDMI_REV)

nit: a nicer look would be something like
	val = readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG);
	if (val == RK3288W_HDMI_REV)

> +		revision = RK3288_SOC_REV_RK3288W;
> +	else
> +		revision = RK3288_SOC_REV_RK3288;
> +
> +	iounmap(hdmi_base);
> +
> +	return revision;
> +}
> +
> +static const char *rk3288_socinfo_revision(u32 rev)
> +{
> +	const char *soc_rev;
> +
> +	switch (rev) {
> +	case RK3288_SOC_REV_RK3288:
> +		soc_rev = "RK3288";
> +		break;
> +
> +	case RK3288_SOC_REV_RK3288W:
> +		soc_rev = "RK3288w";

can we maybe use lower-case letters for all here?

> +		break;
> +
> +	case RK3288_SOC_REV_NOT_DETECT:
> +		soc_rev = "";
> +		break;
> +
> +	default:
> +		soc_rev = "unknown";
> +		break;
> +	}
> +
> +	return kstrdup_const(soc_rev, GFP_KERNEL);
> +}
> +
> +static const struct of_device_id rk3288_soc_match[] = {
> +	{ .compatible = "rockchip,rk3288", },
> +	{ }
> +};
> +
> +static int __init rk3288_soc_init(void)

as noted at the top, I'd really like to see this more generalized so that
other socs can just hook in there with a revision callback in a
rockchip_soc_data struct.


> +{
> +	struct soc_device_attribute *soc_dev_attr;
> +	struct soc_device *soc_dev;
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, rk3288_soc_match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr)
> +		return -ENOMEM;
> +
> +	soc_dev_attr->family = "Rockchip";
> +	soc_dev_attr->soc_id = "RK32xx";

nit: rk3288 instead of "32xx" please

> +
> +	np = of_find_node_by_path("/");
> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
> +	of_node_put(np);
> +
> +	soc_dev_attr->revision = rk3288_socinfo_revision(rk3288_revision());
> +
> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev)) {
> +		kfree_const(soc_dev_attr->revision);
> +		kfree_const(soc_dev_attr->soc_id);
> +		kfree(soc_dev_attr);
> +		return PTR_ERR(soc_dev);
> +	}
> +
> +	dev_info(soc_device_to_device(soc_dev), "Rockchip %s %s detected\n",
> +		 soc_dev_attr->soc_id, soc_dev_attr->revision);

nit: dev_dbg should be enough, that information doesn't really matter for
most people, as it's only relevant to clock internals.


Heiko

Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision

[Mylene Josserand]

Hi Heiko,

On 4/1/20 6:34 PM, Heiko Stübner wrote:
> Hi Mylène,
> 
> Am Mittwoch, 1. April 2020, 17:35:12 CEST schrieb Mylène Josserand:
>> Determine which revision of rk3288 by checking the HDMI version.
>> According to the Rockchip BSP kernel/u-boot, on rk3288w, the HDMI
>> revision equals 0x1A which is not the case for the rk3288.
>>
>> As these SOC have some differences, this driver will help us
>> to know on which revision we are by using 'soc_device' registration
>> to be able to use 'soc_device_match' to detect rk3288/rk3288w.
>>
>> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> 
> I like your new approach quite a lot :-)

Thank you, I am happy to know that :D

> 
> There are some things we need to take into account though, see below.

Sure, no problem

> 
> 
>> ---
>>   drivers/soc/rockchip/Makefile |   1 +
>>   drivers/soc/rockchip/rk3288.c | 125 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 126 insertions(+)
>>   create mode 100644 drivers/soc/rockchip/rk3288.c
>>
>> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
>> index afca0a4c4b72..9dbf12913512 100644
>> --- a/drivers/soc/rockchip/Makefile
>> +++ b/drivers/soc/rockchip/Makefile
>> @@ -2,5 +2,6 @@
>>   #
>>   # Rockchip Soc drivers
>>   #
>> +obj-$(CONFIG_ARCH_ROCKCHIP) += rk3288.o
>>   obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>>   obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
>> diff --git a/drivers/soc/rockchip/rk3288.c b/drivers/soc/rockchip/rk3288.c
> 
> I'd really like this to be a soc.c instead of rk3288.c ;-)

Cool! I will do that :)

> 
> 
>> new file mode 100644
>> index 000000000000..83379ba2b31b
>> --- /dev/null
>> +++ b/drivers/soc/rockchip/rk3288.c
>> @@ -0,0 +1,125 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2020 Collabora Ltd
>> + * Author: Mylene Josserand <mylene.josserand@collabora.com>
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/of_address.h>
>> +#include <linux/sys_soc.h>
>> +#include <linux/slab.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +
>> +#define RK3288_HDMI_REV_REG	0x04
>> +#define RK3288W_HDMI_REV	0x1A
>> +
>> +enum rk3288_soc_rev {
>> +	RK3288_SOC_REV_NOT_DETECT,
>> +	RK3288_SOC_REV_RK3288,
>> +	RK3288_SOC_REV_RK3288W,
>> +};
>> +
>> +static int rk3288_revision(void)
>> +{
>> +	static int revision = RK3288_SOC_REV_NOT_DETECT;
>> +	struct device_node *dn;
>> +	void __iomem *hdmi_base;
>> +
>> +	if (revision != RK3288_SOC_REV_NOT_DETECT)
>> +		return revision;
>> +
>> +	dn = of_find_compatible_node(NULL, NULL, "rockchip,rk3288-dw-hdmi");
>> +	if (!dn) {
>> +		pr_err("%s: Couldn't find HDMI node\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	hdmi_base = of_iomap(dn, 0);
>> +	of_node_put(dn);
>> +
>> +	if (!hdmi_base) {
>> +		pr_err("%s: Couldn't map %pOF regs\n", __func__,
>> +		       hdmi_base);
>> +		return -ENXIO;
>> +	}
> 
> The possible problem I see here is clocking and power-domain of the hdmi
> controller in corner-cases. In the past we already had a lot of fun with
> kexec, which also indicates that people actually use kexec productively.
> 
> So while all clocks are ungated and all power-domains are powered on first
> boot, on a system without graphics the pclk+power-domain could be off when
> doing a kexec into a second kernel, which then would probably hang here.
> 
> 
> Of course with the hdmi-pclk being sourced from hclk_vio we run into a
> chicken-egg-problem, as we need pclk_hdmi_ctrl to register hclk_vio at all.
> 
> So I guess one way out of this could be to
> - amend rk3288_clk_shutdown() to also ungate the hdmi-pclk on shutdown
> - add a shutdown mechanism to the power-domain driver so that it can
>    enable PD_VIO on shutdown

hm, indeed, I will send a v3 including that, thanks for the hints!

> 
>> +
>> +	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG)
>> +	    == RK3288W_HDMI_REV)
> 
> nit: a nicer look would be something like
> 	val = readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG);
> 	if (val == RK3288W_HDMI_REV)

I will change that.

> 
>> +		revision = RK3288_SOC_REV_RK3288W;
>> +	else
>> +		revision = RK3288_SOC_REV_RK3288;
>> +
>> +	iounmap(hdmi_base);
>> +
>> +	return revision;
>> +}
>> +
>> +static const char *rk3288_socinfo_revision(u32 rev)
>> +{
>> +	const char *soc_rev;
>> +
>> +	switch (rev) {
>> +	case RK3288_SOC_REV_RK3288:
>> +		soc_rev = "RK3288";
>> +		break;
>> +
>> +	case RK3288_SOC_REV_RK3288W:
>> +		soc_rev = "RK3288w";
> 
> can we maybe use lower-case letters for all here?

Sure, no problem.

> 
>> +		break;
>> +
>> +	case RK3288_SOC_REV_NOT_DETECT:
>> +		soc_rev = "";
>> +		break;
>> +
>> +	default:
>> +		soc_rev = "unknown";
>> +		break;
>> +	}
>> +
>> +	return kstrdup_const(soc_rev, GFP_KERNEL);
>> +}
>> +
>> +static const struct of_device_id rk3288_soc_match[] = {
>> +	{ .compatible = "rockchip,rk3288", },
>> +	{ }
>> +};
>> +
>> +static int __init rk3288_soc_init(void)
> 
> as noted at the top, I'd really like to see this more generalized so that
> other socs can just hook in there with a revision callback in a
> rockchip_soc_data struct.

Yes, I will do!

> 
> 
>> +{
>> +	struct soc_device_attribute *soc_dev_attr;
>> +	struct soc_device *soc_dev;
>> +	struct device_node *np;
>> +
>> +	np = of_find_matching_node(NULL, rk3288_soc_match);
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>> +	if (!soc_dev_attr)
>> +		return -ENOMEM;
>> +
>> +	soc_dev_attr->family = "Rockchip";
>> +	soc_dev_attr->soc_id = "RK32xx";
> 
> nit: rk3288 instead of "32xx" please

okay

> 
>> +
>> +	np = of_find_node_by_path("/");
>> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
>> +	of_node_put(np);
>> +
>> +	soc_dev_attr->revision = rk3288_socinfo_revision(rk3288_revision());
>> +
>> +	soc_dev = soc_device_register(soc_dev_attr);
>> +	if (IS_ERR(soc_dev)) {
>> +		kfree_const(soc_dev_attr->revision);
>> +		kfree_const(soc_dev_attr->soc_id);
>> +		kfree(soc_dev_attr);
>> +		return PTR_ERR(soc_dev);
>> +	}
>> +
>> +	dev_info(soc_device_to_device(soc_dev), "Rockchip %s %s detected\n",
>> +		 soc_dev_attr->soc_id, soc_dev_attr->revision);
> 
> nit: dev_dbg should be enough, that information doesn't really matter for
> most people, as it's only relevant to clock internals.

yep

Thank you again for this review!

Best regards,
Mylène Josserand

Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision

[Robin Murphy]

On 2020-04-01 5:34 pm, Heiko Stübner wrote:
[...]
> The possible problem I see here is clocking and power-domain of the hdmi
> controller in corner-cases. In the past we already had a lot of fun with
> kexec, which also indicates that people actually use kexec productively.
> 
> So while all clocks are ungated and all power-domains are powered on first
> boot, on a system without graphics the pclk+power-domain could be off when
> doing a kexec into a second kernel, which then would probably hang here.

Just to be that guy, how about kdump, where entry to the second kernel 
is predicated on there *not* being a nice clean shutdown? ;)

IMO relying on any particular bootloader behaviour in the kernel is 
fairly fragile - U-Boot has a lot more latitude in assuming it's running 
straight out of reset than Linux does. If we're not going to trust the 
DT to correctly describe the SoC variant in the first place, then it's 
somewhat questionable whether we should trust it for indirectly 
identifying the SoC variant either - it would seem a lot more robust to 
just map the known physical addresses to run a canned sequence of 
register writes that puts things in a known-good state (on the basis 
that this has to run before the 'real' drivers for those things are up, 
and thus can't interfere with them).

Robin.

> Of course with the hdmi-pclk being sourced from hclk_vio we run into a
> chicken-egg-problem, as we need pclk_hdmi_ctrl to register hclk_vio at all.
> 
> So I guess one way out of this could be to
> - amend rk3288_clk_shutdown() to also ungate the hdmi-pclk on shutdown
> - add a shutdown mechanism to the power-domain driver so that it can
>    enable PD_VIO on shutdown
> 
>> +
>> +	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG)
>> +	    == RK3288W_HDMI_REV)
> 
> nit: a nicer look would be something like
> 	val = readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG);
> 	if (val == RK3288W_HDMI_REV)
> 
>> +		revision = RK3288_SOC_REV_RK3288W;
>> +	else
>> +		revision = RK3288_SOC_REV_RK3288;
>> +
>> +	iounmap(hdmi_base);
>> +
>> +	return revision;
>> +}
>> +
>> +static const char *rk3288_socinfo_revision(u32 rev)
>> +{
>> +	const char *soc_rev;
>> +
>> +	switch (rev) {
>> +	case RK3288_SOC_REV_RK3288:
>> +		soc_rev = "RK3288";
>> +		break;
>> +
>> +	case RK3288_SOC_REV_RK3288W:
>> +		soc_rev = "RK3288w";
> 
> can we maybe use lower-case letters for all here?
> 
>> +		break;
>> +
>> +	case RK3288_SOC_REV_NOT_DETECT:
>> +		soc_rev = "";
>> +		break;
>> +
>> +	default:
>> +		soc_rev = "unknown";
>> +		break;
>> +	}
>> +
>> +	return kstrdup_const(soc_rev, GFP_KERNEL);
>> +}
>> +
>> +static const struct of_device_id rk3288_soc_match[] = {
>> +	{ .compatible = "rockchip,rk3288", },
>> +	{ }
>> +};
>> +
>> +static int __init rk3288_soc_init(void)
> 
> as noted at the top, I'd really like to see this more generalized so that
> other socs can just hook in there with a revision callback in a
> rockchip_soc_data struct.
> 
> 
>> +{
>> +	struct soc_device_attribute *soc_dev_attr;
>> +	struct soc_device *soc_dev;
>> +	struct device_node *np;
>> +
>> +	np = of_find_matching_node(NULL, rk3288_soc_match);
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>> +	if (!soc_dev_attr)
>> +		return -ENOMEM;
>> +
>> +	soc_dev_attr->family = "Rockchip";
>> +	soc_dev_attr->soc_id = "RK32xx";
> 
> nit: rk3288 instead of "32xx" please
> 
>> +
>> +	np = of_find_node_by_path("/");
>> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
>> +	of_node_put(np);
>> +
>> +	soc_dev_attr->revision = rk3288_socinfo_revision(rk3288_revision());
>> +
>> +	soc_dev = soc_device_register(soc_dev_attr);
>> +	if (IS_ERR(soc_dev)) {
>> +		kfree_const(soc_dev_attr->revision);
>> +		kfree_const(soc_dev_attr->soc_id);
>> +		kfree(soc_dev_attr);
>> +		return PTR_ERR(soc_dev);
>> +	}
>> +
>> +	dev_info(soc_device_to_device(soc_dev), "Rockchip %s %s detected\n",
>> +		 soc_dev_attr->soc_id, soc_dev_attr->revision);
> 
> nit: dev_dbg should be enough, that information doesn't really matter for
> most people, as it's only relevant to clock internals.
> 
> 
> Heiko
> 
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision

[Heiko Stübner]

Am Donnerstag, 2. April 2020, 13:45:34 CEST schrieb Robin Murphy:
> On 2020-04-01 5:34 pm, Heiko Stübner wrote:
> [...]
> > The possible problem I see here is clocking and power-domain of the hdmi
> > controller in corner-cases. In the past we already had a lot of fun with
> > kexec, which also indicates that people actually use kexec productively.
> > 
> > So while all clocks are ungated and all power-domains are powered on first
> > boot, on a system without graphics the pclk+power-domain could be off when
> > doing a kexec into a second kernel, which then would probably hang here.
> 
> Just to be that guy, how about kdump, where entry to the second kernel 
> is predicated on there *not* being a nice clean shutdown? ;)
> 
> IMO relying on any particular bootloader behaviour in the kernel is 
> fairly fragile - U-Boot has a lot more latitude in assuming it's running 
> straight out of reset than Linux does.

You'll have to take into account that there are more boot options than
uboot ;-) ... especially the rk3288 also needs to support some ancient
version of coreboot - that definitly won't see any updates anymore
and isn't really user upgradeable.


> If we're not going to trust the 
> DT to correctly describe the SoC variant in the first place,

I'm still all for "just put a rk3288w" into the devicetree compatible,
but so far other participants seem to prefer a software solution ;-) .


> then it's 
> somewhat questionable whether we should trust it for indirectly 
> identifying the SoC variant either - it would seem a lot more robust to 
> just map the known physical addresses to run a canned sequence of 
> register writes that puts things in a known-good state (on the basis 
> that this has to run before the 'real' drivers for those things are up, 
> and thus can't interfere with them).

The problem is, the "known physical address" is part of the dw-hdmi
controller ip block, so we'll also need to take into account clocks and
power-domains.

So that would mean the soc-"driver" would need to 
- ioremap hdmi, cru and pmu
- ungate all clocks (on reboot we don't know the hirarchy)
- enable at least the pd_vio power-domain
via direct register writes.

Doable but definitly very ugly and I also don't really know what more
people farther upstream would say to that.

Anybody interested in just adding that new dt-compatible?


Heiko

> > Of course with the hdmi-pclk being sourced from hclk_vio we run into a
> > chicken-egg-problem, as we need pclk_hdmi_ctrl to register hclk_vio at all.
> > 
> > So I guess one way out of this could be to
> > - amend rk3288_clk_shutdown() to also ungate the hdmi-pclk on shutdown
> > - add a shutdown mechanism to the power-domain driver so that it can
> >    enable PD_VIO on shutdown
> > 
> >> +
> >> +	if (readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG)
> >> +	    == RK3288W_HDMI_REV)
> > 
> > nit: a nicer look would be something like
> > 	val = readl_relaxed(hdmi_base + RK3288_HDMI_REV_REG);
> > 	if (val == RK3288W_HDMI_REV)
> > 
> >> +		revision = RK3288_SOC_REV_RK3288W;
> >> +	else
> >> +		revision = RK3288_SOC_REV_RK3288;
> >> +
> >> +	iounmap(hdmi_base);
> >> +
> >> +	return revision;
> >> +}
> >> +
> >> +static const char *rk3288_socinfo_revision(u32 rev)
> >> +{
> >> +	const char *soc_rev;
> >> +
> >> +	switch (rev) {
> >> +	case RK3288_SOC_REV_RK3288:
> >> +		soc_rev = "RK3288";
> >> +		break;
> >> +
> >> +	case RK3288_SOC_REV_RK3288W:
> >> +		soc_rev = "RK3288w";
> > 
> > can we maybe use lower-case letters for all here?
> > 
> >> +		break;
> >> +
> >> +	case RK3288_SOC_REV_NOT_DETECT:
> >> +		soc_rev = "";
> >> +		break;
> >> +
> >> +	default:
> >> +		soc_rev = "unknown";
> >> +		break;
> >> +	}
> >> +
> >> +	return kstrdup_const(soc_rev, GFP_KERNEL);
> >> +}
> >> +
> >> +static const struct of_device_id rk3288_soc_match[] = {
> >> +	{ .compatible = "rockchip,rk3288", },
> >> +	{ }
> >> +};
> >> +
> >> +static int __init rk3288_soc_init(void)
> > 
> > as noted at the top, I'd really like to see this more generalized so that
> > other socs can just hook in there with a revision callback in a
> > rockchip_soc_data struct.
> > 
> > 
> >> +{
> >> +	struct soc_device_attribute *soc_dev_attr;
> >> +	struct soc_device *soc_dev;
> >> +	struct device_node *np;
> >> +
> >> +	np = of_find_matching_node(NULL, rk3288_soc_match);
> >> +	if (!np)
> >> +		return -ENODEV;
> >> +
> >> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> >> +	if (!soc_dev_attr)
> >> +		return -ENOMEM;
> >> +
> >> +	soc_dev_attr->family = "Rockchip";
> >> +	soc_dev_attr->soc_id = "RK32xx";
> > 
> > nit: rk3288 instead of "32xx" please
> > 
> >> +
> >> +	np = of_find_node_by_path("/");
> >> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
> >> +	of_node_put(np);
> >> +
> >> +	soc_dev_attr->revision = rk3288_socinfo_revision(rk3288_revision());
> >> +
> >> +	soc_dev = soc_device_register(soc_dev_attr);
> >> +	if (IS_ERR(soc_dev)) {
> >> +		kfree_const(soc_dev_attr->revision);
> >> +		kfree_const(soc_dev_attr->soc_id);
> >> +		kfree(soc_dev_attr);
> >> +		return PTR_ERR(soc_dev);
> >> +	}
> >> +
> >> +	dev_info(soc_device_to_device(soc_dev), "Rockchip %s %s detected\n",
> >> +		 soc_dev_attr->soc_id, soc_dev_attr->revision);
> > 
> > nit: dev_dbg should be enough, that information doesn't really matter for
> > most people, as it's only relevant to clock internals.
> > 
> > 
> > Heiko
> > 
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > 
> 

[PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w

[Mylène Josserand]

The revision rk3288w has a different clock tree about
"hclk_vio" clock, according to the BSP kernel code.

This patch handles this difference by detecting which SOC it is
and creating the div accordingly. Because we are using
soc_device_match function, we need to delay the registration
of this clock later than others to have time to get SoC revision.

Otherwise, because of CLK_OF_DECLARE uses, clock tree will be
created too soon to have time to detect SoC's revision.

Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
---
 drivers/clk/rockchip/clk-rk3288.c | 36 +++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index cc2a177bbdbf..1950d1efe1b8 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -9,6 +9,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/syscore_ops.h>
+#include <linux/sys_soc.h>
 #include <dt-bindings/clock/rk3288-cru.h>
 #include "clk.h"
 
@@ -425,8 +426,6 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 	COMPOSITE(0, "aclk_vio0", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
 			RK3288_CLKSEL_CON(31), 6, 2, MFLAGS, 0, 5, DFLAGS,
 			RK3288_CLKGATE_CON(3), 0, GFLAGS),
-	DIV(0, "hclk_vio", "aclk_vio0", 0,
-			RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
 	COMPOSITE(0, "aclk_vio1", mux_pll_src_cpll_gpll_usb480m_p, CLK_IGNORE_UNUSED,
 			RK3288_CLKSEL_CON(31), 14, 2, MFLAGS, 8, 5, DFLAGS,
 			RK3288_CLKGATE_CON(3), 2, GFLAGS),
@@ -819,6 +818,16 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 	INVERTER(0, "pclk_isp", "pclk_isp_in", RK3288_CLKSEL_CON(29), 3, IFLAGS),
 };
 
+static struct rockchip_clk_branch rk3288w_hclkvio_branch[] __initdata = {
+	DIV(0, "hclk_vio", "aclk_vio1", 0,
+	    RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
+};
+
+static struct rockchip_clk_branch rk3288_hclkvio_branch[] __initdata = {
+	DIV(0, "hclk_vio", "aclk_vio0", 0,
+	    RK3288_CLKSEL_CON(28), 8, 5, DFLAGS),
+};
+
 static const char *const rk3288_critical_clocks[] __initconst = {
 	"aclk_cpu",
 	"aclk_peri",
@@ -914,10 +923,15 @@ static struct syscore_ops rk3288_clk_syscore_ops = {
 	.resume = rk3288_clk_resume,
 };
 
+static const struct soc_device_attribute rk3288w[] = {
+	{ .soc_id = "RK32xx", .revision = "RK3288w" },
+	{ /* sentinel */ }
+};
+
+static struct rockchip_clk_provider *ctx;
+
 static void __init rk3288_clk_init(struct device_node *np)
 {
-	struct rockchip_clk_provider *ctx;
-
 	rk3288_cru_base = of_iomap(np, 0);
 	if (!rk3288_cru_base) {
 		pr_err("%s: could not map cru region\n", __func__);
@@ -955,3 +969,17 @@ static void __init rk3288_clk_init(struct device_node *np)
 	rockchip_clk_of_add_provider(np, ctx);
 }
 CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288-cru", rk3288_clk_init);
+
+static int __init rk3288_hclkvio_register(void)
+{
+	/* Check for the rk3288w revision as clock tree is different */
+	if (soc_device_match(rk3288w))
+		rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
+					       ARRAY_SIZE(rk3288w_hclkvio_branch));
+	else
+		rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
+					       ARRAY_SIZE(rk3288_hclkvio_branch));
+
+	return 0;
+}
+subsys_initcall(rk3288_hclkvio_register);
-- 
2.25.1

Re: [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w

[Geert Uytterhoeven]

Hi Mylène,

On Wed, Apr 1, 2020 at 5:35 PM Mylène Josserand
<mylene.josserand@collabora.com> wrote:
> The revision rk3288w has a different clock tree about
> "hclk_vio" clock, according to the BSP kernel code.
>
> This patch handles this difference by detecting which SOC it is
> and creating the div accordingly. Because we are using
> soc_device_match function, we need to delay the registration
> of this clock later than others to have time to get SoC revision.
>
> Otherwise, because of CLK_OF_DECLARE uses, clock tree will be
> created too soon to have time to detect SoC's revision.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>

Thanks for your patch!

> --- a/drivers/clk/rockchip/clk-rk3288.c
> +++ b/drivers/clk/rockchip/clk-rk3288.c
> @@ -914,10 +923,15 @@ static struct syscore_ops rk3288_clk_syscore_ops = {
>         .resume = rk3288_clk_resume,
>  };
>
> +static const struct soc_device_attribute rk3288w[] = {
> +       { .soc_id = "RK32xx", .revision = "RK3288w" },
> +       { /* sentinel */ }
> +};
> +
> +static struct rockchip_clk_provider *ctx;
> +
>  static void __init rk3288_clk_init(struct device_node *np)
>  {
> -       struct rockchip_clk_provider *ctx;
> -
>         rk3288_cru_base = of_iomap(np, 0);
>         if (!rk3288_cru_base) {
>                 pr_err("%s: could not map cru region\n", __func__);
> @@ -955,3 +969,17 @@ static void __init rk3288_clk_init(struct device_node *np)
>         rockchip_clk_of_add_provider(np, ctx);
>  }
>  CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288-cru", rk3288_clk_init);
> +
> +static int __init rk3288_hclkvio_register(void)
> +{

This function will always be called, even when running a (multi-platform)
kernel on a non-rk3288 platform.  So you need some protection against
that.

> +       /* Check for the rk3288w revision as clock tree is different */
> +       if (soc_device_match(rk3288w))
> +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
> +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
> +       else
> +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
> +                                              ARRAY_SIZE(rk3288_hclkvio_branch));

Note that soc_device_match() returns a struct soc_device_attribute
pointer.  If you would store the rockchip_clk_branch array pointer and
size in rk3288w[...].data (i.e. a pointer to a struct containing that
info), for both the r83288w and normal rk3288 variants, you could
simplify this to:

    attr = soc_device_match(rk3288w);
    if (attr) {
            struct rk3288_branch_array *p = attr->data;
            rockchip_clk_register_branches(ctx, p->branches, p->len);
    }

That would handle the not-running-on-rk3288 issue as well.

> +
> +       return 0;
> +}
> +subsys_initcall(rk3288_hclkvio_register);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w

[Mylene Josserand]

Hi Geert,

On 4/1/20 6:56 PM, Geert Uytterhoeven wrote:
> Hi Mylène,
> 
> On Wed, Apr 1, 2020 at 5:35 PM Mylène Josserand
> <mylene.josserand@collabora.com> wrote:
>> The revision rk3288w has a different clock tree about
>> "hclk_vio" clock, according to the BSP kernel code.
>>
>> This patch handles this difference by detecting which SOC it is
>> and creating the div accordingly. Because we are using
>> soc_device_match function, we need to delay the registration
>> of this clock later than others to have time to get SoC revision.
>>
>> Otherwise, because of CLK_OF_DECLARE uses, clock tree will be
>> created too soon to have time to detect SoC's revision.
>>
>> Signed-off-by: Mylène Josserand <mylene.josserand@collabora.com>
> 
> Thanks for your patch!

Thanks for your review!

> 
>> --- a/drivers/clk/rockchip/clk-rk3288.c
>> +++ b/drivers/clk/rockchip/clk-rk3288.c
>> @@ -914,10 +923,15 @@ static struct syscore_ops rk3288_clk_syscore_ops = {
>>          .resume = rk3288_clk_resume,
>>   };
>>
>> +static const struct soc_device_attribute rk3288w[] = {
>> +       { .soc_id = "RK32xx", .revision = "RK3288w" },
>> +       { /* sentinel */ }
>> +};
>> +
>> +static struct rockchip_clk_provider *ctx;
>> +
>>   static void __init rk3288_clk_init(struct device_node *np)
>>   {
>> -       struct rockchip_clk_provider *ctx;
>> -
>>          rk3288_cru_base = of_iomap(np, 0);
>>          if (!rk3288_cru_base) {
>>                  pr_err("%s: could not map cru region\n", __func__);
>> @@ -955,3 +969,17 @@ static void __init rk3288_clk_init(struct device_node *np)
>>          rockchip_clk_of_add_provider(np, ctx);
>>   }
>>   CLK_OF_DECLARE(rk3288_cru, "rockchip,rk3288-cru", rk3288_clk_init);
>> +
>> +static int __init rk3288_hclkvio_register(void)
>> +{
> 
> This function will always be called, even when running a (multi-platform)
> kernel on a non-rk3288 platform.  So you need some protection against
> that.

erg, good point, I didn't think about that.

> 
>> +       /* Check for the rk3288w revision as clock tree is different */
>> +       if (soc_device_match(rk3288w))
>> +               rockchip_clk_register_branches(ctx, rk3288w_hclkvio_branch,
>> +                                              ARRAY_SIZE(rk3288w_hclkvio_branch));
>> +       else
>> +               rockchip_clk_register_branches(ctx, rk3288_hclkvio_branch,
>> +                                              ARRAY_SIZE(rk3288_hclkvio_branch));
> 
> Note that soc_device_match() returns a struct soc_device_attribute
> pointer.  If you would store the rockchip_clk_branch array pointer and
> size in rk3288w[...].data (i.e. a pointer to a struct containing that
> info), for both the r83288w and normal rk3288 variants, you could
> simplify this to:
> 
>      attr = soc_device_match(rk3288w);
>      if (attr) {
>              struct rk3288_branch_array *p = attr->data;
>              rockchip_clk_register_branches(ctx, p->branches, p->len);
>      }
> 
> That would handle the not-running-on-rk3288 issue as well.

Nice, thank you for the explanation and the code, very useful :)

Best regards,

Mylène