[RESEND PATCH] soc/qbman: Disable IRQs for deferred QBMan work

[Roy Pledge]

Work for Congestion State Notifications (CSCN) and Message Ring (MR)
handling is handled via the workqueue mechanism. This requires the
driver to disable those IRQs before scheduling the work and re-enabling
it once the work is completed so that the interrupt doesn't continually
fire.

Signed-off-by: Roy Pledge 
---
 drivers/soc/fsl/qbman/qman.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index d67b8e1..f1a242a 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1382,6 +1382,7 @@ static void qm_congestion_task(struct work_struct *work)
if (!qm_mc_result_timeout(>p, )) {
spin_unlock(>cgr_lock);
dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
+   qman_p_irqsource_add(p, QM_PIRQ_CSCI);
return;
}
/* mask out the ones I'm not interested in */
@@ -1396,6 +1397,7 @@ static void qm_congestion_task(struct work_struct *work)
if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
spin_unlock(>cgr_lock);
+   qman_p_irqsource_add(p, QM_PIRQ_CSCI);
 }
 
 static void qm_mr_process_task(struct work_struct *work)
@@ -1455,12 +1457,14 @@ static void qm_mr_process_task(struct work_struct *work)
}
 
qm_mr_cci_consume(>p, num);
+   qman_p_irqsource_add(p, QM_PIRQ_MRI);
preempt_enable();
 }
 
 static u32 __poll_portal_slow(struct qman_portal *p, u32 is)
 {
if (is & QM_PIRQ_CSCI) {
+   qman_p_irqsource_remove(p, QM_PIRQ_CSCI);
queue_work_on(smp_processor_id(), qm_portal_wq,
  >congestion_work);
}
@@ -1472,6 +1476,7 @@ static u32 __poll_portal_slow(struct qman_portal *p, u32 
is)
}
 
if (is & QM_PIRQ_MRI) {
+   qman_p_irqsource_remove(p, QM_PIRQ_MRI);
queue_work_on(smp_processor_id(), qm_portal_wq,
  >mr_work);
}
-- 
1.7.9.5

Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file

[Steven Rostedt]

On Fri, 10 Mar 2017 21:38:53 +0530
"Naveen N. Rao"  wrote:

> On 2017/03/10 10:45AM, Steven Rostedt wrote:
> > On Thu, 02 Mar 2017 20:38:53 +1100
> > Michael Ellerman  wrote:

> > > So if we drop that we're left with ftrace.S - which seems perfect to me.  
> > 
> > Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S
> > will get the same ftrace.o. Maybe make it ftrace-hook.S ?  
> 
> I've avoided that issue by naming the files ftrace_32.S and ftrace_64.S 
> (which gets further split up).

That's what I looked at doing for x86 as well. But not all archs have
32 / 64 splits. Should we look to have something that all archs can be
consistent with?

-- Steve

Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file

[Naveen N. Rao]

On 2017/03/10 10:45AM, Steven Rostedt wrote:
> On Thu, 02 Mar 2017 20:38:53 +1100
> Michael Ellerman  wrote:
> 
> > Steven Rostedt  writes:
> > 
> > > On Tue, 28 Feb 2017 15:04:15 +1100
> > > Michael Ellerman  wrote:
> > >
> > > kernel/trace/ftrace.c more obvious.  
> > >> 
> > >> I don't know if it's really worth keeping the names the same across
> > >> arches, especially as we already have:
> > >> 
> > >>   arch/arm64/kernel/entry-ftrace.S
> > >>   arch/arm/kernel/entry-ftrace.S
> > >>   arch/blackfin/kernel/ftrace-entry.S
> > >>   arch/metag/kernel/ftrace_stub.S
> > >> 
> > >> But we can rename it if you feel strongly about it.  
> > >
> > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked
> > > the "mcount.S" name.  
> > 
> > Except what does the "entry" part mean?
> > 
> > Traditionally entry.S has been for the code that "enters" the kernel,
> > ie. from userspace or elsewhere. But that's not the case with any of the
> > ftrace code, it's kernel code called from the kernel. So using "entry"
> > is a bit wrong IMHO.
> > 
> > So if we drop that we're left with ftrace.S - which seems perfect to me.
> 
> Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S
> will get the same ftrace.o. Maybe make it ftrace-hook.S ?

I've avoided that issue by naming the files ftrace_32.S and ftrace_64.S 
(which gets further split up).

Thanks,
Naveen

Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file

[Steven Rostedt]

On Thu, 02 Mar 2017 20:38:53 +1100
Michael Ellerman  wrote:

> Steven Rostedt  writes:
> 
> > On Tue, 28 Feb 2017 15:04:15 +1100
> > Michael Ellerman  wrote:
> >
> > kernel/trace/ftrace.c more obvious.  
> >> 
> >> I don't know if it's really worth keeping the names the same across
> >> arches, especially as we already have:
> >> 
> >>   arch/arm64/kernel/entry-ftrace.S
> >>   arch/arm/kernel/entry-ftrace.S
> >>   arch/blackfin/kernel/ftrace-entry.S
> >>   arch/metag/kernel/ftrace_stub.S
> >> 
> >> But we can rename it if you feel strongly about it.  
> >
> > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked
> > the "mcount.S" name.  
> 
> Except what does the "entry" part mean?
> 
> Traditionally entry.S has been for the code that "enters" the kernel,
> ie. from userspace or elsewhere. But that's not the case with any of the
> ftrace code, it's kernel code called from the kernel. So using "entry"
> is a bit wrong IMHO.
> 
> So if we drop that we're left with ftrace.S - which seems perfect to me.

Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S
will get the same ftrace.o. Maybe make it ftrace-hook.S ?

-- Steve

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Segher Boessenkool]

On Fri, Mar 10, 2017 at 03:41:23PM +0100, Christophe LEROY wrote:
> gpio_get() and gpio_set() are used extensively by some GPIO based
> drivers like SPI, NAND, so it may be worth it as it doesn't impair
> readability (if anyone prefers, we could write  (1 << 31) >> i  instead
> of  0x8000 >> i )
> >>>
> >>>1 << 31 is undefined behaviour, of course.
> >>
> >>Shall it be 1U << 31 ?
> >
> >Sure, that works.  "1 << (31 - i)" is most readable (but it doesn't yet
> >generate the code you want).
> 
> Euh  I'm a bit lost. Do you mean the form we have today is the 
> driver is wrong ?

Heh, yes.  But is't okay with GCC, so don't worry about it.

The point is that "0x8000 >> i" is less readable.


Segher

Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file

[Naveen N. Rao]

On 2017/03/02 08:38PM, Michael Ellerman wrote:
> Steven Rostedt  writes:
> 
> > On Tue, 28 Feb 2017 15:04:15 +1100
> > Michael Ellerman  wrote:
> >
> > kernel/trace/ftrace.c more obvious.
> >> 
> >> I don't know if it's really worth keeping the names the same across
> >> arches, especially as we already have:
> >> 
> >>   arch/arm64/kernel/entry-ftrace.S
> >>   arch/arm/kernel/entry-ftrace.S
> >>   arch/blackfin/kernel/ftrace-entry.S
> >>   arch/metag/kernel/ftrace_stub.S
> >> 
> >> But we can rename it if you feel strongly about it.
> >
> > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked
> > the "mcount.S" name.
> 
> Except what does the "entry" part mean?
> 
> Traditionally entry.S has been for the code that "enters" the kernel,
> ie. from userspace or elsewhere. But that's not the case with any of the
> ftrace code, it's kernel code called from the kernel. So using "entry"
> is a bit wrong IMHO.
> 
> So if we drop that we're left with ftrace.S - which seems perfect to me.

Hi Steve,
Are you ok with this? I'd prefer to not add the 'entry-' prefix too, 
seeing as it will make the file names quite long without necessarily 
adding much.

Thanks,
Naveen

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Christophe LEROY]

Le 10/03/2017 à 15:32, Segher Boessenkool a écrit :

On Fri, Mar 10, 2017 at 03:04:48PM +0100, Christophe LEROY wrote:

Le 10/03/2017 à 14:06, Segher Boessenkool a écrit :

On Fri, Mar 10, 2017 at 11:54:19AM +0100, Christophe LEROY wrote:

gpio_get() and gpio_set() are used extensively by some GPIO based
drivers like SPI, NAND, so it may be worth it as it doesn't impair
readability (if anyone prefers, we could write  (1 << 31) >> i  instead
of  0x8000 >> i )


1 << 31 is undefined behaviour, of course.



Shall it be 1U << 31 ?


Sure, that works.  "1 << (31 - i)" is most readable (but it doesn't yet
generate the code you want).




Euh  I'm a bit lost. Do you mean the form we have today is the 
driver is wrong ?



@@ -684,9 +682,7 @@ static int cpm1_gpio32_get(struct gpio_chip *gc, 
unsigned int gpio)

 {
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
struct cpm_ioport32b __iomem *iop = mm_gc->regs;
-   u32 pin_mask;
-
-   pin_mask = 1 << (31 - gpio);
+   u32 pin_mask = 0x8000 >> gpio;

return !!(in_be32(>dat) & pin_mask);
 }


Which I thought could also become


@@ -684,9 +682,7 @@ static int cpm1_gpio32_get(struct gpio_chip *gc, 
unsigned int gpio)

 {
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
struct cpm_ioport32b __iomem *iop = mm_gc->regs;
-   u32 pin_mask;
-
-   pin_mask = 1 << (31 - gpio);
+   u32 pin_mask = (1 << 31) >> gpio;

return !!(in_be32(>dat) & pin_mask);
 }


Christophe

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Segher Boessenkool]

On Fri, Mar 10, 2017 at 03:04:48PM +0100, Christophe LEROY wrote:
> Le 10/03/2017 à 14:06, Segher Boessenkool a écrit :
> >On Fri, Mar 10, 2017 at 11:54:19AM +0100, Christophe LEROY wrote:
> >>gpio_get() and gpio_set() are used extensively by some GPIO based
> >>drivers like SPI, NAND, so it may be worth it as it doesn't impair
> >>readability (if anyone prefers, we could write  (1 << 31) >> i  instead
> >>of  0x8000 >> i )
> >
> >1 << 31 is undefined behaviour, of course.
> >
> 
> Shall it be 1U << 31 ?

Sure, that works.  "1 << (31 - i)" is most readable (but it doesn't yet
generate the code you want).


Segher

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Christophe LEROY]

Le 10/03/2017 à 14:06, Segher Boessenkool a écrit :

On Fri, Mar 10, 2017 at 11:54:19AM +0100, Christophe LEROY wrote:

gpio_get() and gpio_set() are used extensively by some GPIO based
drivers like SPI, NAND, so it may be worth it as it doesn't impair
readability (if anyone prefers, we could write  (1 << 31) >> i  instead
of  0x8000 >> i )


1 << 31 is undefined behaviour, of course.



Shall it be 1U << 31 ?

Christophe

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Segher Boessenkool]

On Fri, Mar 10, 2017 at 11:54:19AM +0100, Christophe LEROY wrote:
> gpio_get() and gpio_set() are used extensively by some GPIO based 
> drivers like SPI, NAND, so it may be worth it as it doesn't impair 
> readability (if anyone prefers, we could write  (1 << 31) >> i  instead 
> of  0x8000 >> i )

1 << 31 is undefined behaviour, of course.


Segher

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Segher Boessenkool]

On Fri, Mar 10, 2017 at 07:41:33PM +1100, Michael Ellerman wrote:
> Well yeah, it saves one instruction, but is it worth it? Are these gpio
> routines in some hot path I don't know about?

If there was a GCC PR for this we probably would make GCC optimise
this; there are many similar things that are optimised already, just
not this one.


Segher

[PATCH v4] powernv/sensor: Handle OPAL_WRONG_STATE error return

[Vipin K Parashar]

OPAL returns OPAL_WRONG_STATE upon failing to provide
sensor data due to core sleeping/offline. Added check
in opal_get_sensor_data() for sensor read failure with
OPAL_WRONG_STATE return code and returned -EIO.

Signed-off-by: Vipin K Parashar 
---
Changes in v4:
 - Removed sleeping core log message with KERN_NOTICE priority.

Changes in v3:
 - Added a new case for OPAL_WRONG_STATE in sensor read
   along with a log message indicating sleeping/offline core
   causing read fail.

 arch/powerpc/platforms/powernv/opal-sensor.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c 
b/arch/powerpc/platforms/powernv/opal-sensor.c
index 308efd1..aa267f1 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -64,6 +64,10 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)
*sensor_data = be32_to_cpu(data);
break;
 
+   case OPAL_WRONG_STATE:
+   ret = -EIO;
+   break;
+
default:
ret = opal_error_code(ret);
break;
-- 
2.7.4

[PATCH 3.16 122/370] powerpc: Fix build warning on 32-bit PPC

[Ben Hutchings]

3.16.42-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Larry Finger 

commit 8ae679c4bc2ea2d16d92620da8e3e9332fa4039f upstream.

I am getting the following warning when I build kernel 4.9-git on my
PowerBook G4 with a 32-bit PPC processor:

AS  arch/powerpc/kernel/misc_32.o
  arch/powerpc/kernel/misc_32.S:299:7: warning: "CONFIG_FSL_BOOKE" is not 
defined [-Wundef]

This problem is evident after commit 989cea5c14be ("kbuild: prevent
lib-ksyms.o rebuilds"); however, this change in kbuild only exposes an
error that has been in the code since 2005 when this source file was
created.  That was with commit 9994a33865f4 ("powerpc: Introduce
entry_{32,64}.S, misc_{32,64}.S, systbl.S").

The offending line does not make a lot of sense.  This error does not
seem to cause any errors in the executable, thus I am not recommending
that it be applied to any stable versions.

Thanks to Nicholas Piggin for suggesting this solution.

Fixes: 9994a33865f4 ("powerpc: Introduce entry_{32,64}.S, misc_{32,64}.S, 
systbl.S")
Signed-off-by: Larry Finger 
Cc: Nicholas Piggin 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Linus Torvalds 
Signed-off-by: Ben Hutchings 
---
 arch/powerpc/kernel/misc_32.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -313,7 +313,7 @@ _GLOBAL(flush_instruction_cache)
lis r3, KERNELBASE@h
iccci   0,r3
 #endif
-#elif CONFIG_FSL_BOOKE
+#elif defined(CONFIG_FSL_BOOKE)
 BEGIN_FTR_SECTION
mfspr   r3,SPRN_L1CSR0
ori r3,r3,L1CSR0_CFI|L1CSR0_CLFC

[RFC] powerpc: handle simultanneous interrupts at once

[Christophe Leroy]

It often happens to have simultanneous interrupts, for instance
when having double Ethernet attachment. With the current
implementation, we suffer the cost of kernel entry/exit for each
interrupt.

This patch introduces a loop in __do_irq() to handle all interrupts
at once before returning.

Signed-off-by: Christophe Leroy 
---
 arch/powerpc/include/asm/hw_irq.h |  6 ++
 arch/powerpc/kernel/irq.c | 22 +++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_irq.h 
b/arch/powerpc/include/asm/hw_irq.h
index eba60416536e..d69ae5846955 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -123,6 +123,11 @@ static inline void may_hard_irq_enable(void)
__hard_irq_enable();
 }
 
+static inline void may_hard_irq_disable(void)
+{
+   __hard_irq_disable();
+}
+
 static inline bool arch_irq_disabled_regs(struct pt_regs *regs)
 {
return !regs->softe;
@@ -204,6 +209,7 @@ static inline bool arch_irq_disabled_regs(struct pt_regs 
*regs)
 }
 
 static inline void may_hard_irq_enable(void) { }
+static inline void may_hard_irq_disable(void) { }
 
 #endif /* CONFIG_PPC64 */
 
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index a018f5cae899..28aca510c166 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -515,14 +515,22 @@ void __do_irq(struct pt_regs *regs)
 */
irq = ppc_md.get_irq();
 
-   /* We can hard enable interrupts now to allow perf interrupts */
-   may_hard_irq_enable();
+   do {
+   /* We can hard enable interrupts now to allow perf interrupts */
+   may_hard_irq_enable();
+
+   /* And finally process it */
+   if (unlikely(!irq))
+   __this_cpu_inc(irq_stat.spurious_irqs);
+   else
+   generic_handle_irq(irq);
+
+   may_hard_irq_disable();
 
-   /* And finally process it */
-   if (unlikely(!irq))
-   __this_cpu_inc(irq_stat.spurious_irqs);
-   else
-   generic_handle_irq(irq);
+   irq = ppc_md.get_irq();
+   } while (irq);
+
+   may_hard_irq_enable();
 
trace_irq_exit(regs);
 
-- 
2.12.0

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Christophe LEROY]

Le 10/03/2017 à 09:41, Michael Ellerman a écrit :

Christophe Leroy  writes:


Help a bit the compiler to provide better code:

unsigned int f(int i)
{
return 1 << (31 - i);
}

unsigned int g(int i)
{
return 0x8000 >> i;
}

Disassembly of section .text:

 :
   0:   20 63 00 1f subfic  r3,r3,31
   4:   39 20 00 01 li  r9,1
   8:   7d 23 18 30 slw r3,r9,r3
   c:   4e 80 00 20 blr

0010 :
  10:   3d 20 80 00 lis r9,-32768
  14:   7d 23 1c 30 srw r3,r9,r3
  18:   4e 80 00 20 blr


Well yeah, it saves one instruction, but is it worth it? Are these gpio
routines in some hot path I don't know about?



It saves one instruction, and one register (see other exemple below 
where r3 is to be preserved)


gpio_get() and gpio_set() are used extensively by some GPIO based 
drivers like SPI, NAND, so it may be worth it as it doesn't impair 
readability (if anyone prefers, we could write  (1 << 31) >> i  instead 
of  0x8000 >> i )


unsigned int f(int i, unsigned int *a)
{
*a = 1 << (31 - i);

return i;
}

unsigned int g(int i, unsigned int *a)
{
*a = 0x8000 >> i;

return i;
}

toto.o: file format elf32-powerpc


Disassembly of section .text:

 :
   0:   21 43 00 1f subfic  r10,r3,31
   4:   39 20 00 01 li  r9,1
   8:   7d 29 50 30 slw r9,r9,r10
   c:   91 24 00 00 stw r9,0(r4)
  10:   4e 80 00 20 blr

0014 :
  14:   3d 20 80 00 lis r9,-32768
  18:   7d 29 1c 30 srw r9,r9,r3
  1c:   91 24 00 00 stw r9,0(r4)
  20:   4e 80 00 20 blr

Re: [PATCH 1/2] powerpc/fadump: reserve memory at halfway mark

[Michael Ellerman]

Hari Bathini  writes:

> Currently, the area to preserve boot memory is reserved at the top of
> RAM. This leaves fadump vulnerable to DLPAR memory remove operations.
> As memory for fadump needs to be reserved early in the boot process,
> fadump can't be registered after a DLPAR memory remove operation.
> While this problem can't be eleminated completely, the impact can be
> minimized by reserving memory at the halfway mark instead. With this
> change, fadump can register successfully after a DLPAR memory remove
> operation as long as the sum of the sizes of boot memory and memory
> removed is less than half of the total available memory.
>
> Signed-off-by: Hari Bathini 
> ---
>  arch/powerpc/kernel/fadump.c |8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 8ff0dd4..9c85c5a 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -319,9 +319,13 @@ int __init fadump_reserve_mem(void)
>   pr_debug("fadumphdr_addr = %p\n",
>   (void *) fw_dump.fadumphdr_addr);
>   } else {
> - /* Reserve the memory at the top of memory. */
> + /*
> +  * Reserve memory at the halfway mark to minimize
> +  * the impact of DLPAR memory remove operation.
> +  */
> + base = PAGE_ALIGN(memory_boundary/2);

This doesn't account for holes, do we never have holes in the memory
layout on phyp?

cheers

[PATCH] powerpc/8xx: fix mpc8xx_get_irq() return on no irq

[Christophe Leroy]

IRQ 0 is a valid HW interrupt. So get_irq() shall return 0 when
there is no irq, instead of returning irq_linear_revmap(... ,0)

Fixes: f2a0bd3753dad ("[POWERPC] 8xx: powerpc port of core CPM PIC")
Signed-off-by: Christophe Leroy 
---
 arch/powerpc/sysdev/mpc8xx_pic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
index 3e828b20c21e..2842f9d63d21 100644
--- a/arch/powerpc/sysdev/mpc8xx_pic.c
+++ b/arch/powerpc/sysdev/mpc8xx_pic.c
@@ -79,7 +79,7 @@ unsigned int mpc8xx_get_irq(void)
irq = in_be32(_reg->sc_sivec) >> 26;
 
if (irq == PIC_VEC_SPURRIOUS)
-   irq = 0;
+   return 0;
 
 return irq_linear_revmap(mpc8xx_pic_host, irq);
 
-- 
2.12.0

Re: [PATCH 5/5] powerpc/64s: fix POWER9 machine check handler from stop state

[Gautham R Shenoy]

On Wed, Mar 08, 2017 at 10:19:19PM +1000, Nicholas Piggin wrote:
> Hi Gautham,
> 
> I'm just getting back to this. Sorry for the late reply, and
> thanks for the reviews.

No problems.

> 
[..snip..]
> > > +#ifdef CONFIG_PPC_P7_NAP
> > > +EXC_COMMON_BEGIN(machine_check_idle_common)
> > > +   bl  machine_check_queue_event
> > > +   /*
> > > +* Queue the machine check, then reload SRR1 and use it to set
> > > +* CR3 according to pnv_powersave_wakeup convention.
> > > +*/
> > > +   ld  r12,_MSR(r1)
> > > +   rlwinm  r11,r12,47-31,30,31
> > > +   cmpwi   cr3,r11,2
> > > +
> > > +   /*
> > > +* Now have to make SRR1 wake up reason look like a system reset
> > > +* interrupt. Put 0xf in there, which is reserved (and does not
> > > +* match HMI).  
> > 
> > The only places where the wakeup reason is presently checked on the way out
> > of idle-exit are in KVM guest entry path in kvmppc_check_wake_reason()
> >  and on the CPU-Hotplug exit path pnv_smp_cpu_kill_self(). In both places
> > we do a positive check for EE, Doorbell, HVEE . The kvm case is also
> > interested in
> > HMI. We ignore all the other reasons at the moment.
> > 
> > So this should be fine.
> 
> Okay, thanks for confirming. We will have to be careful about this I
> suppose if the wakeup reasons are expanded. I'll make a note of it
> in asm/reg.h with the WAKEMASK definitions.

Sounds reasonable.

> 
> > > +*/
> > > +   li  r11,0xf
> > > +   insrdi  r12,r11,4,45  
> > 
> > 
> > Shouldn't this be insrdi r12,r11,4,42? The exception bits are 42:45.
> > I always have trouble wrapping my head around these nifty
> > rotate-shift-mask-insert instructions. So I might as well be wrong!
> 
> Ah I think you're right, good catch. Maybe oris r12,r12,0x3c is a better
> choice than that insrdi?

Perhaps oris is a better choice in this case since we are anyway
setting every bit in 42:45 range. Not sure if it will save any cycles,
but it will certainly reduce an instruction!

> 
> 
> > 
> > Otherwise, the patch looks correct to me.
> > Reviewed-by: Gautham R. Shenoy 
> 
> Very much appreciate the reviews. I'm just getting some time to work on
> the winkle count patch, so I'll repost with your suggestions when that's
> done.
>

Looking forward to the new version!


> Thanks,
> Nick
> 
--
Thanks and Regards
gautham.

Re: [PATCH] powerpc: sysdev: cpm1: Optimise gpio bit calculation

[Michael Ellerman]

Christophe Leroy  writes:

> Help a bit the compiler to provide better code:
>
> unsigned int f(int i)
> {
>   return 1 << (31 - i);
> }
>
> unsigned int g(int i)
> {
>   return 0x8000 >> i;
> }
>
> Disassembly of section .text:
>
>  :
>0: 20 63 00 1f subfic  r3,r3,31
>4: 39 20 00 01 li  r9,1
>8: 7d 23 18 30 slw r3,r9,r3
>c: 4e 80 00 20 blr
>
> 0010 :
>   10: 3d 20 80 00 lis r9,-32768
>   14: 7d 23 1c 30 srw r3,r9,r3
>   18: 4e 80 00 20 blr

Well yeah, it saves one instruction, but is it worth it? Are these gpio
routines in some hot path I don't know about?

cheers

Re: [PATCH] powerpc: asm: convert directive .llong to .8byte

[Tobin C. Harding]

On Fri, Mar 10, 2017 at 06:40:58PM +1100, Michael Ellerman wrote:
> Daniel Axtens  writes:
> 
> > Hi Tobin,
> >
> >> .llong is an undocumented PPC specific directive. The generic
> >> equivalent is .quad, but even better (because it's self describing) is
> >> .8byte.
> >>
> >> Convert directives .llong -> .8byte
> >>
> >> Signed-off-by: Tobin C. Harding 
> >> ---
> >>
> >> Fixes: issue #33 (github)
> >
> > Thanks for tackling these!
> >
> > I have applied your patch to my local tree. I ran `git grep '\.llong'`,
> > and found:
> >
> > tools/testing/selftests/powerpc/switch_endian/switch_endian_test.S: .llong 
> > 0x
> >
> > That file is also handled by mpe and the powerpc tree even though it
> > doesn't live in arch/powerpc - could you please change that one as well?
> 
> I can do that one when I apply it.

Excellent.

thanks,
Tobin.

Re: [PATCH] powerpc: asm: convert directive .llong to .8byte

[Tobin C. Harding]

On Fri, Mar 10, 2017 at 11:09:08AM +1100, Daniel Axtens wrote:
> Hi Tobin,
> 
> > .llong is an undocumented PPC specific directive. The generic
> > equivalent is .quad, but even better (because it's self describing) is
> > .8byte.
> >
> > Convert directives .llong -> .8byte
> >
> > Signed-off-by: Tobin C. Harding 
> > ---
> >
> > Fixes: issue #33 (github)
> 
> Thanks for tackling these!
> 
> I have applied your patch to my local tree. I ran `git grep '\.llong'`,
> and found:
> 
> tools/testing/selftests/powerpc/switch_endian/switch_endian_test.S: .llong 
> 0x
> 
> That file is also handled by mpe and the powerpc tree even though it
> doesn't live in arch/powerpc - could you please change that one as well?

Awesome, thanks Daniel. I did not know to look there. Will re submit.

thanks,
Tobin.