Re: [RFC PATCH v2 30/31] kvx: Add power controller driver

[Krzysztof Kozlowski]

On 15/04/2024 16:08, Yann Sionneau wrote:
> Hello Krzysztof, Arnd, all,
> 
> On 1/22/23 12:54, Krzysztof Kozlowski wrote:
>> On 20/01/2023 15:10, Yann Sionneau wrote:
>>> From: Jules Maselbas <jmaselbas@xxxxxxxxx>
>>>
>>> The Power Controller (pwr-ctrl) control cores reset and wake-up
>>> procedure.
>>> +
>>> +int __init kvx_pwr_ctrl_probe(void)
>>> +{
>>> +	struct device_node *ctrl;
>>> +
>>> +	ctrl = get_pwr_ctrl_node();
>>> +	if (!ctrl) {
>>> +		pr_err("Failed to get power controller node\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-ctrl")) {
>>> +		pr_err("Failed to get power controller node\n");
>> No. Drivers go to drivers, not to arch directory. This should be a
>> proper driver instead of some fake stub doing its own driver matching.
>> You need to rework this.
> 
> I am working on a v3 patchset, therefore I am working on a solution for 
> this "pwr-ctrl" driver that needs to go somewhere else than arch/kvx/.
> 
> The purpose of this "driver" is just to expose a void 
> kvx_pwr_ctrl_cpu_poweron(unsigned int cpu) function, used by 
> kernel/smpboot.c function __cpu_up() in order to start secondary CPUs in 
> SMP config.

I might be missing here some bigger picture and maybe my original
comment was no appropriate, but IIUC, you might now create dependencies
between arch code and drivers. That's also fragile.

> 
> Doing this, on our SoC, requires writing 3 registers in a memory-mapped 
> device named "power controller".
> 
> I made some researches in drivers/ but I am not sure yet what's a good 
> place that fits what our device is doing (booting secondary CPUs).
> 
> * drivers/power/reset seems to be for resetting the entire SoC
> 
> * drivers/power/supply seems to be to control power supplies ICs/periph.
> 
> * drivers/reset seems to be for device reset
> 
> * drivers/pmdomain maybe ?
> 
> * drivers/soc ?
> 

Bringup of CPU? Then I would vote for here. You also have existing
example: r9a06g032-smp.c

But anyway the point is to make it clear - either it is a driver or core
code. Not both. The original code was not looking like any other CPU
bringup code.

Best regards,
Krzysztof