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

["Heiko Stübner" <heiko@sntech.de>]

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
> > 
>