Re: [PATCH V2 16/19] csky: SMP support

[Mark Rutland]

On Mon, Jul 02, 2018 at 01:30:19AM +0800, Guo Ren wrote:
> +static int csky_of_cpu(struct device_node *node)
> +{
> +	const char *status;
> +	int cpu;
> +
> +	if (of_property_read_u32(node, "reg", &cpu))
> +		goto error;
> +
> +	if (cpu >= NR_CPUS)
> +		goto error;
> +
> +	if (of_property_read_string(node, "status", &status))
> +		status = "enable";
> +
> +	if (strcmp(status, "disable") == 0)
> +		goto error;

Please use of_device_is_available(node); "enable" is not a sensible value for
the status property, and "disable" (rather than "disabled") is simply unusual.

Neither "enable" nor "disable" are correct values for the status property.

What is the value in the reg property, exactly? Is there a unique ID in
hardware for each CPU in the system?

It would be good to document this, e.g. as arm does in
Documentation/devicetree/bindings/arm/cpus.txt

> +
> +	return cpu;
> +error:
> +	return -ENODEV;
> +}
> +
> +void __init setup_smp(void)
> +{
> +	struct device_node *node = NULL;
> +	int cpu;
> +
> +	while ((node = of_find_node_by_type(node, "cpu"))) {
> +		cpu = csky_of_cpu(node);
> +		if (cpu >= 0) {
> +			set_cpu_possible(cpu, true);
> +			set_cpu_present(cpu, true);
> +		}
> +	}
> +}

What happens if/when the value in the reg property is larger than NR_CPUS?

> +int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> +{
> +	unsigned int tmp;
> +
> +	secondary_stack = (unsigned int)tidle->stack + THREAD_SIZE;
> +
> +	secondary_hint = mfcr("cr31");
> +
> +	secondary_ccr  = mfcr("cr18");
> +
> +	pr_info("%s: CPU%u\n", __func__, cpu);
> +
> +	tmp = mfcr("cr<29, 0>");
> +	tmp |= 1 << cpu;
> +	mtcr("cr<29, 0>", tmp);
> +
> +	while (!cpu_online(cpu));
> +
> +	secondary_stack = 0;
> +
> +	return 0;
> +}

I don't see a start address being setup here, so I assume that CPUs branch to a
fixed address out-of-reset. Does that mean that the kernel has to be loaded at
a particular physical address on a given platform?

Thanks,
Mark.