Messages in this thread | | | Subject | Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision | From | Mylene Josserand <> | Date | Thu, 2 Apr 2020 08:31:10 +0200 |
| |
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
| |