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

[Robin Murphy <robin.murphy@arm.com>]

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
>