* [PATCH v2 0/2] ARM: Add Rockchip rk3288w support @ 2020-04-01 15:35 Mylène Josserand 2020-04-01 15:35 ` [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision Mylène Josserand 2020-04-01 15:35 ` [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w Mylène Josserand 0 siblings, 2 replies; 9+ messages in thread From: Mylène Josserand @ 2020-04-01 15:35 UTC (permalink / raw) To: heiko, linux-arm-kernel, mturquette, sboyd Cc: linux-kernel, linux-rockchip, linux-clk, mylene.josserand, kernel, kever.yang, geert 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision 2020-04-01 15:35 [PATCH v2 0/2] ARM: Add Rockchip rk3288w support Mylène Josserand @ 2020-04-01 15:35 ` Mylène Josserand 2020-04-01 16:34 ` Heiko Stübner 2020-04-01 15:35 ` [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w Mylène Josserand 1 sibling, 1 reply; 9+ messages in thread From: Mylène Josserand @ 2020-04-01 15:35 UTC (permalink / raw) To: heiko, linux-arm-kernel, mturquette, sboyd Cc: linux-kernel, linux-rockchip, linux-clk, mylene.josserand, kernel, kever.yang, geert 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision 2020-04-01 15:35 ` [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision Mylène Josserand @ 2020-04-01 16:34 ` Heiko Stübner 2020-04-02 6:31 ` Mylene Josserand 2020-04-02 11:45 ` Robin Murphy 0 siblings, 2 replies; 9+ messages in thread From: Heiko Stübner @ 2020-04-01 16:34 UTC (permalink / raw) To: Mylène Josserand Cc: linux-arm-kernel, mturquette, sboyd, linux-kernel, linux-rockchip, linux-clk, kernel, kever.yang, geert 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision 2020-04-01 16:34 ` Heiko Stübner @ 2020-04-02 6:31 ` Mylene Josserand 2020-04-02 11:45 ` Robin Murphy 1 sibling, 0 replies; 9+ messages in thread From: Mylene Josserand @ 2020-04-02 6:31 UTC (permalink / raw) To: Heiko Stübner Cc: linux-arm-kernel, mturquette, sboyd, linux-kernel, linux-rockchip, linux-clk, kernel, kever.yang, geert 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision 2020-04-01 16:34 ` Heiko Stübner 2020-04-02 6:31 ` Mylene Josserand @ 2020-04-02 11:45 ` Robin Murphy 2020-04-02 14:14 ` Heiko Stübner 1 sibling, 1 reply; 9+ messages in thread From: Robin Murphy @ 2020-04-02 11:45 UTC (permalink / raw) To: Heiko Stübner, Mylène Josserand Cc: sboyd, mturquette, linux-kernel, kever.yang, linux-rockchip, geert, kernel, linux-clk, linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision 2020-04-02 11:45 ` Robin Murphy @ 2020-04-02 14:14 ` Heiko Stübner 0 siblings, 0 replies; 9+ messages in thread From: Heiko Stübner @ 2020-04-02 14:14 UTC (permalink / raw) To: Robin Murphy Cc: Mylène Josserand, sboyd, mturquette, linux-kernel, kever.yang, linux-rockchip, geert, kernel, linux-clk, linux-arm-kernel 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w 2020-04-01 15:35 [PATCH v2 0/2] ARM: Add Rockchip rk3288w support Mylène Josserand 2020-04-01 15:35 ` [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision Mylène Josserand @ 2020-04-01 15:35 ` Mylène Josserand 2020-04-01 16:56 ` Geert Uytterhoeven 1 sibling, 1 reply; 9+ messages in thread From: Mylène Josserand @ 2020-04-01 15:35 UTC (permalink / raw) To: heiko, linux-arm-kernel, mturquette, sboyd Cc: linux-kernel, linux-rockchip, linux-clk, mylene.josserand, kernel, kever.yang, geert 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w 2020-04-01 15:35 ` [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w Mylène Josserand @ 2020-04-01 16:56 ` Geert Uytterhoeven 2020-04-02 6:35 ` Mylene Josserand 0 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2020-04-01 16:56 UTC (permalink / raw) To: Mylène Josserand Cc: Heiko Stuebner, Linux ARM, Michael Turquette, Stephen Boyd, Linux Kernel Mailing List, open list:ARM/Rockchip SoC..., linux-clk, Collabora Kernel ML, kever.yang 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w 2020-04-01 16:56 ` Geert Uytterhoeven @ 2020-04-02 6:35 ` Mylene Josserand 0 siblings, 0 replies; 9+ messages in thread From: Mylene Josserand @ 2020-04-02 6:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Heiko Stuebner, Linux ARM, Michael Turquette, Stephen Boyd, Linux Kernel Mailing List, open list:ARM/Rockchip SoC..., linux-clk, Collabora Kernel ML, kever.yang 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-02 14:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-01 15:35 [PATCH v2 0/2] ARM: Add Rockchip rk3288w support Mylène Josserand 2020-04-01 15:35 ` [PATCH v2 1/2] soc: rockchip: Register a soc_device to retrieve revision Mylène Josserand 2020-04-01 16:34 ` Heiko Stübner 2020-04-02 6:31 ` Mylene Josserand 2020-04-02 11:45 ` Robin Murphy 2020-04-02 14:14 ` Heiko Stübner 2020-04-01 15:35 ` [PATCH v2 2/2] clk: rockchip: rk3288: Handle clock tree for rk3288w Mylène Josserand 2020-04-01 16:56 ` Geert Uytterhoeven 2020-04-02 6:35 ` Mylene Josserand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).