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

[heiko@sntech.de (Heiko Stuebner)]

Hi Shawn,

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

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

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

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


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

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

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

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


Heiko