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

[Pavel Machek]

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