Re: [RFC PATCH v3 34/37] kvx: Add power controller driver

Yann Sionneau
2024-08-23T13:10:04Z

Hello Krzysztof,

On 22/07/2024 14:37, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
>> From: Yann Sionneau <ysionneau@kalrayinc.com>
>>
>> The Power Controller (pwr-ctrl) controls cores reset and wake-up
>> procedure.
>> [...]
>> +
>> +static bool pwr_ctrl_not_initialized = true;
> Do not use inverted meanings.
Ack, I will fix this.
>
>> +
>> +/**
>> + * kvx_pwr_ctrl_cpu_poweron() - Wakeup a cpu
>> + * @cpu: cpu to wakeup
>> + */
>> +int __init kvx_pwr_ctrl_cpu_poweron(unsigned int cpu)
>> +{
>> + int ret = 0;
>> +
>> + if (pwr_ctrl_not_initialized) {
>> + pr_err("KVX power controller not initialized!\n");
>> + return -ENODEV;
>> + }
>> +
>> + /* Set PE boot address */
>> + writeq((unsigned long long)kvx_start,
> Addresses use kernel_ulong_t
Ack, I will fix this.
>
>> + kvx_pwr_controller.regs + KVX_PWR_CTRL_RESET_PC_OFFSET);
>> + /* Wake up processor ! */
>> + writeq(1ULL << cpu,
> That's BIT
Ack, I will fix this and replace with BITULL(cpu).
>
>> + kvx_pwr_controller.regs + PWR_CTRL_WUP_SET_OFFSET);
>> + /* Then clear wakeup to allow processor to sleep */
>> + writeq(1ULL << cpu,
> BIT
Ack.
>
>> + kvx_pwr_controller.regs + PWR_CTRL_WUP_CLEAR_OFFSET);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct smp_operations coolidge_smp_ops __initconst = {
>> + .smp_boot_secondary = kvx_pwr_ctrl_cpu_poweron,
>> +};
>> +
>> +static int __init kvx_pwr_ctrl_probe(void)
> That's not a probe, please rename to avoid confusion. Or make it a
> proper device driver.

Ok, I will probably rename it kvx_pwr_ctrl_init()

Thanks!

--

Yann