Re: [PATCH 1/1] nds32: Power management for nds32

From: Pavel Machek
Date: Wed Oct 24 2018 - 02:51:23 EST


On Wed 2018-10-24 11:40:07, Nickhu wrote:
> There are three sleep states in nds32:
> suspend to idle,
> suspend to standby,
> suspend to ram
>
> In suspend to ram, we use the 'standby' instruction to emulate
> power management device to hang the system util wakeup source
> send wakeup events to break the loop.
>
> First, we push the general purpose registers and system registers
> to stack. Second, we translate stack pointer to physical address
> and store to memory to save the stack pointer. Third, after write
> back and invalid the cache we hang in 'standby' intruction.
> When wakeup source trigger wake up events, the loop will be break
> and resume the system.
>
> Signed-off-by: Nickhu <nickhu@xxxxxxxxxxxxx>

Is "Nickhu" complete name?

> diff --git a/arch/nds32/kernel/pm.c b/arch/nds32/kernel/pm.c
> new file mode 100644
> index 000000000000..e1eaf3bac709
> --- /dev/null
> +++ b/arch/nds32/kernel/pm.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2008-2017 Andes Technology Corporation
> +
> +/*
> + * nds32 Power Management Routines
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License.
> + *
> + * Abstract:
> + *
> + * This program is for nds32 power management routines.
> + *
> + */

I'd get rid of "abstract" here, repeating GPL twice and "nds32 power
management routines" twice does not make much sense either.

> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2017 Andes Technology Corporation */
> +
> +#include<asm/memory.h>

Missing space.

> +static int nointc_set_wake(struct irq_data *data, unsigned int on)
> +{
> + unsigned long int_mask = __nds32__mfsr(NDS32_SR_INT_MASK);
> + static bool is_bit_1[NR_IRQS] = {false};
> + u32 bit = 1 << data->hwirq;
> +
> + if (on) {
> + if (int_mask & bit)
> + is_bit_1[data->hwirq] = true;
> + else
> + is_bit_1[data->hwirq] = false;
> +
> + int_mask |= bit;
> + wake_mask |= bit;
> + } else {
> + if (!is_bit_1[data->hwirq])
> + int_mask &= ~bit;
> +
> + wake_mask &= ~bit;
> + is_bit_1[data->hwirq] = false;
> + }

Can we get rid of "is_bit_1" array here, and use normal bit operations
on another variable here?

Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature