RE: [4/4] powerpc/mpc8548: Add workaround for erratum NMG_SRIO135

2013-10-17 Thread Zhao Chenhui-B35336

OK. I will do.

-Chenhui


From: Wood Scott-B07421
Sent: Thursday, October 17, 2013 7:20
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [4/4] powerpc/mpc8548: Add workaround for erratum NMG_SRIO135

On Tue, Mar 06, 2012 at 05:10:56PM +0800, chenhui zhao wrote:
 From: chenhui zhao chenhui.z...@freescale.com

 Issue:
 Applications using lwarx/stwcx instructions in the core to
 compete for a software lock or semaphore with a device on
 RapidIO using read atomic set, clr, inc, or dec in a similar
 manner may falsely result in both masters seeing the lock
 as available. This could result in data corruption as
 both masters try to modify the same piece of data protected
 by the lock.

 Workaround:
 Set bits 13 and 29 of CCSR offset 0x01010 (EEBPCR register
 of the ECM) during initialization and leave them set
 indefinitely. This may slightly degrade overall system
 performance.

 Refer to SRIO39 in MPC8548 errata document.

 Signed-off-by: Gong Chen g.c...@freescale.com
 Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
 Signed-off-by: Li Yang le...@freescale.com

 ---
 arch/powerpc/sysdev/fsl_rio.c |   44 +
  1 files changed, 44 insertions(+), 0 deletions(-)
[snip]
 @@ -358,6 +391,17 @@ int fsl_rio_setup(struct platform_device *dev)
   dev-dev.of_node-full_name);
   return -EFAULT;
   }
 +
 + /* Fix erratum NMG_SRIO135 */
 + if (fsl_svr_is(SVR_8548) || fsl_svr_is(SVR_8548_E)) {
 + rc = fixup_erratum_srio135(dev-dev);
 + if (rc) {
 + dev_err(dev-dev,
 + Failed to fix the erratum NMG_SRIO135.);
 + return rc;
 + }
 + }

This needs to be respun based on the current tree.

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH 02/17] powerpc/85xx: do not sync time base at boot time

2013-04-03 Thread Zhao Chenhui-B35336

No other reason. Just avoid doing it again at boot time in kernel.

-Chenhui


From: Kumar Gala [ga...@kernel.crashing.org]
Sent: Wednesday, April 03, 2013 23:10
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 02/17] powerpc/85xx: do not sync time base at boot time

On Apr 3, 2013, at 8:09 AM, Zhao Chenhui wrote:

 From: Chen-Hui Zhao chenhui.z...@freescale.com

 The bootloader have done time base sync for all cores, so skip
 the synchronization process at boot time of kernel.

 Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
 Signed-off-by: Li Yang le...@freescale.com
 Signed-off-by: Andy Fleming aflem...@freescale.com
 ---
 arch/powerpc/platforms/85xx/smp.c |8 
 1 files changed, 8 insertions(+), 0 deletions(-)

What harm is there in doing the sync?  I'm sure there is another reason you 
want to skip the TB sync that should be conveyed in the commit message.

- k

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH v7 2/5] powerpc/85xx: add HOTPLUG_CPU support

2012-07-17 Thread Zhao Chenhui-B35336
 -Original Message-
 From: Kumar Gala [mailto:ga...@kernel.crashing.org]
 Sent: Friday, July 13, 2012 8:15 PM
 To: Zhao Chenhui-B35336
 Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; 
 linux-ker...@vger.kernel.org; Li Yang-R58472
 Subject: Re: [PATCH v7 2/5] powerpc/85xx: add HOTPLUG_CPU support
 
 
 On Jul 3, 2012, at 5:21 AM, Zhao Chenhui wrote:
 
  From: Li Yang le...@freescale.com
 
  Add support to disable and re-enable individual cores at runtime
  on MPC85xx/QorIQ SMP machines. Currently support e500v1/e500v2 core.
 
  MPC85xx machines use ePAPR spin-table in boot page for CPU kick-off.
  This patch uses the boot page from bootloader to boot core at runtime.
  It supports 32-bit and 36-bit physical address.
 
  Add generic_set_cpu_up() to set cpu_state as CPU_UP_PREPARE in kick_cpu().
 
 Shouldn't the generic_setup_cpu_up() be a separate patch, and refactor 
 smp_generic_kick_cpu() to use
 it.
 
 Also, we should pull the conversion of the spintable from #defines to struct 
 into a separate patch
 before this one.
 

Ok. I will split this patch.

-Chenhui

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH v6 1/5] powerpc/85xx: implement hardware timebase sync

2012-06-29 Thread Zhao Chenhui-B35336
 -Original Message-
 From: Linuxppc-dev 
 [mailto:linuxppc-dev-bounces+chenhui.zhao=freescale@lists.ozlabs.org] On 
 Behalf
 Of Kumar Gala
 Sent: Friday, June 29, 2012 2:30 AM
 To: Zhao Chenhui-B35336
 Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org list; 
 linux-ker...@vger.kernel.org list
 Subject: Re: [PATCH v6 1/5] powerpc/85xx: implement hardware timebase sync
 
 
 On Jun 28, 2012, at 5:50 AM, Benjamin Herrenschmidt wrote:
 
  On Thu, 2012-06-28 at 11:38 +0800, Zhao Chenhui wrote:
 
 
  The bootloader have done a timebase sync. If we do not need KEXEC or
  HOTPLUG_CPU feature, it is unnecessary to do it again at boot time of
  kernel. I only compile the timebase sync routines
  when users enable KEXEC or HOTPLUG_CPU.
 
  Still, how much are you really saving ? Is it worth the added mess and
  loss of test coverage ?
 
  We have too many conditional stuff like that already.
 
  Cheers,
  Ben.
 
 
 I'd also be interested to know how long it actually takes to do time base 
 sync this way.  Since you
 are freezing the timers for some period how long does it really take between 
 the freeze/unfreeze in
 mpc85xx_give_timebase()
 
 + mpc85xx_timebase_freeze(1);
 ...
 + mpc85xx_timebase_freeze(0);
 
 You can use ATBL/U as a way to see # of cycles taken.
 
 - k

I measured it using ATBL on MPC8572DS with 1.5GHz core frequency and 600MHz CCB 
frequency.
The average of 10 times is 1019 clock. It seems that most of the time spent by 
isync and msync.

-Chenhui

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


power management patch set for mpc85xx

2012-06-01 Thread Zhao Chenhui-B35336
Hi Ben and Paul,

I am sorry to trouble you. It seems that Kumar is busy recently.

Could you have a review on the following patches? These patches
implement the power management support on MPC85xx platform.

http://patchwork.ozlabs.org/patch/158484/
http://patchwork.ozlabs.org/patch/158485/
http://patchwork.ozlabs.org/patch/158487/
http://patchwork.ozlabs.org/patch/158486/
http://patchwork.ozlabs.org/patch/158488/

Thanks.

Best Regards,
Chenhui


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [linuxppc-release] [PATCH v5 1/5] powerpc/85xx: implement hardware timebase sync

2012-05-29 Thread Zhao Chenhui-B35336
Hi Kumar,

There is no comment for these patches so far. Do you think these patches can be 
merged?
We really want these patches to be merged in this merge window.

Thanks.

Best Regards,
Chenhui


 -Original Message-
 From: Zhao Chenhui-B35336
 Sent: Friday, May 25, 2012 3:09 PM
 To: Wood Scott-B07421; ga...@kernel.crashing.org
 Cc: Li Yang-R58472
 Subject: RE: [linuxppc-release] [PATCH v5 1/5] powerpc/85xx: implement 
 hardware timebase sync
 
 Hi Scott and Kumar,
 
 Do you have comments for these patches?
 
 http://patchwork.ozlabs.org/patch/158484/
 http://patchwork.ozlabs.org/patch/158485/
 http://patchwork.ozlabs.org/patch/158487/
 http://patchwork.ozlabs.org/patch/158486/
 http://patchwork.ozlabs.org/patch/158488/
 
 Thanks.
 
 Best Regards,
 Chenhui
 
  -Original Message-
  From: linuxppc-release-boun...@linux.freescale.net [mailto:linuxppc-release-
  boun...@linux.freescale.net] On Behalf Of Zhao Chenhui-B35336
  Sent: Friday, May 11, 2012 7:54 PM
  To: linuxppc-dev@lists.ozlabs.org
  Cc: Wood Scott-B07421; Li Yang-R58472; linux-ker...@vger.kernel.org; 
  ga...@kernel.crashing.org
  Subject: [linuxppc-release] [PATCH v5 1/5] powerpc/85xx: implement hardware 
  timebase sync
 
  Do hardware timebase sync. Firstly, stop all timebases, and transfer
  the timebase value of the boot core to the other core. Finally,
  start all timebases.
 
  Only apply to dual-core chips, such as MPC8572, P2020, etc.
 
  Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
  Signed-off-by: Li Yang le...@freescale.com
  ---
   arch/powerpc/include/asm/fsl_guts.h |2 +
   arch/powerpc/platforms/85xx/smp.c   |   93 
  +--
   2 files changed, 91 insertions(+), 4 deletions(-)
 
  diff --git a/arch/powerpc/include/asm/fsl_guts.h 
  b/arch/powerpc/include/asm/fsl_guts.h
  index aa4c488..dd5ba2c 100644
  --- a/arch/powerpc/include/asm/fsl_guts.h
  +++ b/arch/powerpc/include/asm/fsl_guts.h
  @@ -48,6 +48,8 @@ struct ccsr_guts {
   __be32  dmuxcr;/* 0x.0068 - DMA Mux Control Register */
   u8 res06c[0x70 - 0x6c];
  __be32  devdisr;/* 0x.0070 - Device Disable Control */
  +#define CCSR_GUTS_DEVDISR_TB1  0x1000
  +#define CCSR_GUTS_DEVDISR_TB0  0x4000
  __be32  devdisr2;   /* 0x.0074 - Device Disable Control 2 */
  u8  res078[0x7c - 0x78];
  __be32  pmjcr;  /* 0x.007c - 4 Power Management Jog Control 
  Register */
  diff --git a/arch/powerpc/platforms/85xx/smp.c 
  b/arch/powerpc/platforms/85xx/smp.c
  index ff42490..6862dda 100644
  --- a/arch/powerpc/platforms/85xx/smp.c
  +++ b/arch/powerpc/platforms/85xx/smp.c
  @@ -24,6 +24,7 @@
   #include asm/mpic.h
   #include asm/cacheflush.h
   #include asm/dbell.h
  +#include asm/fsl_guts.h
 
   #include sysdev/fsl_soc.h
   #include sysdev/mpic.h
  @@ -115,13 +116,70 @@ smp_85xx_kick_cpu(int nr)
 
   struct smp_ops_t smp_85xx_ops = {
  .kick_cpu = smp_85xx_kick_cpu,
  -#ifdef CONFIG_KEXEC
  -   .give_timebase  = smp_generic_give_timebase,
  -   .take_timebase  = smp_generic_take_timebase,
  -#endif
   };
 
   #ifdef CONFIG_KEXEC
  +static struct ccsr_guts __iomem *guts;
  +static u64 timebase;
  +static int tb_req;
  +static int tb_valid;
  +
  +static void mpc85xx_timebase_freeze(int freeze)
  +{
  +   unsigned int mask;
  +
  +   if (!guts)
  +   return;
  +
  +   mask = CCSR_GUTS_DEVDISR_TB0 | CCSR_GUTS_DEVDISR_TB1;
  +   if (freeze)
  +   setbits32(guts-devdisr, mask);
  +   else
  +   clrbits32(guts-devdisr, mask);
  +
  +   in_be32(guts-devdisr);
  +}
  +
  +static void mpc85xx_give_timebase(void)
  +{
  +   unsigned long flags;
  +
  +   local_irq_save(flags);
  +
  +   while (!tb_req)
  +   barrier();
  +   tb_req = 0;
  +
  +   mpc85xx_timebase_freeze(1);
  +   timebase = get_tb();
  +   mb();
  +   tb_valid = 1;
  +
  +   while (tb_valid)
  +   barrier();
  +
  +   mpc85xx_timebase_freeze(0);
  +
  +   local_irq_restore(flags);
  +}
  +
  +static void mpc85xx_take_timebase(void)
  +{
  +   unsigned long flags;
  +
  +   local_irq_save(flags);
  +
  +   tb_req = 1;
  +   while (!tb_valid)
  +   barrier();
  +
  +   set_tb(timebase  32, timebase  0x);
  +   mb();
  +   tb_valid = 0;
  +
  +   local_irq_restore(flags);
  +}
  +
   atomic_t kexec_down_cpus = ATOMIC_INIT(0);
 
   void mpc85xx_smp_kexec_cpu_down(int crash_shutdown, int secondary)
  @@ -228,6 +286,20 @@ smp_85xx_setup_cpu(int cpu_nr)
  doorbell_setup_this_cpu();
   }
 
  +#ifdef CONFIG_KEXEC
  +static const struct of_device_id guts_ids[] = {
  +   { .compatible = fsl,mpc8572-guts, },
  +   { .compatible = fsl,mpc8560-guts, },
  +   { .compatible = fsl,mpc8536-guts, },
  +   { .compatible = fsl,p1020-guts, },
  +   { .compatible = fsl,p1021-guts, },
  +   { .compatible = fsl,p1022-guts, },
  +   { .compatible = fsl,p1023-guts, },
  +   { .compatible = fsl,p2020-guts, },
  +   {},
  +};
  +#endif
  +
   void

RE: [PATCH 2/4] fsl_pci: Add a workaround for PCI 5 errata in MPC8548

2012-03-08 Thread Zhao Chenhui-B35336
 On Mar 6, 2012, at 3:10 AM, Zhao Chenhui wrote:
 
  +   if ((fsl_svr_is(SVR_8548) || fsl_svr_is(SVR_8548_E)) 
 
 Should this also have 8547, 8547E, 8545, 8545E, 8543,  8543E?

Yes. I will include these chips.

-Chenhui

 
  +   !early_find_capability(hose, 0, 0, PCI_CAP_ID_PCIX)) {
  +   early_read_config_word(hose, 0, 0,
  +   PCI_BUS_FUNCTION, temp);
  +   temp |= PCI_BUS_FUNCTION_MDS;
  +   early_write_config_word(hose, 0, 0,
  +   PCI_BUS_FUNCTION, temp);
  +   }
  }
 


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH 1/9] pci_ids: Add device ID for IBM PCI-X bridge

2012-03-07 Thread Zhao Chenhui-B35336
 On Mar 6, 2012, at 3:05 AM, Zhao Chenhui wrote:
 
  Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
  ---
  include/linux/pci_ids.h |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
 
 Just merge this with the 2nd patch that actually uses the ID.
 
 - k

Ok. I put it in the file mpc85xx_cds.c.

-Chenhui

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH 2/9] powerpc/mpc85xxcds: Fix PCI I/O space resource of PCI bridge

2012-03-07 Thread Zhao Chenhui-B35336
  diff --git a/arch/powerpc/platforms/85xx/mpc85xx_cds.c 
  b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
  index 40f03da..c009c5b 100644
  --- a/arch/powerpc/platforms/85xx/mpc85xx_cds.c
  +++ b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
  @@ -3,7 +3,7 @@
   *
   * Maintained by Kumar Gala (see MAINTAINERS for contact information)
   *
  - * Copyright 2005 Freescale Semiconductor Inc.
  + * Copyright 2005, 2011-2012 Freescale Semiconductor Inc.
   *
   * This program is free software; you can redistribute  it and/or modify it
   * under  the terms of  the GNU General  Public License as published by the
  @@ -158,6 +158,31 @@ DECLARE_PCI_FIXUP_EARLY(0x1957, 0x3fff, 
  skip_fake_bridge);
  DECLARE_PCI_FIXUP_EARLY(0x3fff, 0x1957, skip_fake_bridge);
  DECLARE_PCI_FIXUP_EARLY(0xff3f, 0x5719, skip_fake_bridge);
 
  +/*
  + * Fix Tsi310 PCI-X bridge resource.
  + * Force the bridge to open a window from 0x-0x1fff in PCI I/O space.
  + * This allows legacy I/O(i8259, etc) on the VIA southbridge to be 
  accessed.
  + */
 
 This comment and the code don't make sense.  Why is the bridge described as 
 Tsi310 in comments but the
 vendor ID is IBM ?

This chip is from IBM originally, and bought by IDT.
The vendor ID is IBM, but the part number is Tsi310(IDT).

-Chenhui

 
  +void mpc85xx_cds_fixup_bus(struct pci_bus *bus)
  +{
  +   struct pci_dev *dev = bus-self;
  +   struct resource *res = bus-resource[0];
  +
  +   if (dev != NULL 
  +   dev-vendor == PCI_VENDOR_ID_IBM 
  +   dev-device == PCI_DEVICE_ID_IBM_PCIX_BRIDGE) {
  +   if (res) {
  +   res-start = 0;
  +   res-end   = 0x1fff;
  +   res-flags = IORESOURCE_IO;
  +   pr_info(mpc85xx_cds: PCI bridge resource fixup 
  applied\n);
  +   pr_info(mpc85xx_cds: %pR\n, res);
  +   }
  +   }
  +
  +   fsl_pcibios_fixup_bus(bus);
  +}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


RE: [PATCH v3] powerpc/85xx: add support to JOG feature using cpufreq interface

2012-01-04 Thread Zhao Chenhui-B35336
 On 12/27/2011 05:25 AM, Zhao Chenhui wrote:
   * The driver doesn't support MPC8536 Rev 1.0 due to a JOG erratum.
 Subsequent revisions of MPC8536 have corrected the erratum.
 
 Where do you check for this?

Nowhere. I just notify this patch don't support MPC8536 Rev 1.0.

 
  +#define POWMGTCSR_LOSSLESS_MASK0x0040
  +#define POWMGTCSR_JOG_MASK 0x0020
 
 Are these really masks, or just values to use?

They are masks.

 
  +#define POWMGTCSR_CORE0_IRQ_MSK0x8000
  +#define POWMGTCSR_CORE0_CI_MSK 0x4000
  +#define POWMGTCSR_CORE0_DOZING 0x0008
  +#define POWMGTCSR_CORE0_NAPPING0x0004
  +
  +#define POWMGTCSR_CORE_INT_MSK 0x0800
  +#define POWMGTCSR_CORE_CINT_MSK0x0400
  +#define POWMGTCSR_CORE_UDE_MSK 0x0200
  +#define POWMGTCSR_CORE_MCP_MSK 0x0100
  +#define P1022_POWMGTCSR_MSK(POWMGTCSR_CORE_INT_MSK | \
  +POWMGTCSR_CORE_CINT_MSK | \
  +POWMGTCSR_CORE_UDE_MSK | \
  +POWMGTCSR_CORE_MCP_MSK)
  +
  +static void keep_waking_up(void *dummy)
  +{
  +   unsigned long flags;
  +
  +   local_irq_save(flags);
  +   mb();
  +
  +   in_jog_process = 1;
  +   mb();
  +
  +   while (in_jog_process != 0)
  +   mb();
  +
  +   local_irq_restore(flags);
  +}
 
 Please document this.  Compare in_jog_process == 1, not != 0 -- it's
 unlikely, but what if the other cpu sees that in_jog_process has been
 set to 1, exits and sets in_jog_process to 0, then re-enters set_pll and
 sets in_jog_process to -1 again before this function does another load
 of in_jog_process?

Thanks. I'll fix it.

 
 Do you really need all these mb()s?  I think this would suffice:
 
   local_irq_save(flags);
 
   in_jog_process = 1;
 
   while (in_jog_process == 1)
   barrier();
 
   local_irq_restore();
 
 It's not really a performance issue, just simplicity.
 
  +static int p1022_set_pll(unsigned int cpu, unsigned int pll)
  +{
  +   int index, hw_cpu = get_hard_smp_processor_id(cpu);
  +   int shift;
  +   u32 corefreq, val, mask = 0;
  +   unsigned int cur_pll = get_pll(hw_cpu);
  +   unsigned long flags;
  +   int ret = 0;
  +
  +   if (pll == cur_pll)
  +   return 0;
  +
  +   shift = hw_cpu * CORE_RATIO_BITS + CORE0_RATIO_SHIFT;
  +   val = (pll  CORE_RATIO_MASK)  shift;
  +
  +   corefreq = sysfreq * pll / 2;
  +   /*
  +* Set the COREx_SPD bit if the requested core frequency
  +* is larger than the threshold frequency.
  +*/
  +   if (corefreq  FREQ_533MHz)
  +   val |= PMJCR_CORE0_SPD_MASK  hw_cpu;
 
 P1022 manual says the threshold is 500 MHz (but doesn't say how to set
 the bit if the frequency is exactly 500 MHz).  Where did 53334 come
 from?

Please refer to Chapter 25 25.4.1.11 Power Management Jog Control Register 
(PMJCR).

 
  +
  +   mask = (CORE_RATIO_MASK  shift) | (PMJCR_CORE0_SPD_MASK 
 hw_cpu);
  +   clrsetbits_be32(guts + PMJCR, mask, val);
  +
  +   /* readback to sync write */
  +   val = in_be32(guts + PMJCR);
 
 You don't use val after this -- just ignore the return value from
 in_be32().

OK.

 
  +   /*
  +* A Jog request can not be asserted when any core is in a low
  +* power state on P1022. Before executing a jog request, any
  +* core which is in a low power state must be waked by a
  +* interrupt, and keep waking up until the sequence is
  +* finished.
  +*/
  +   for_each_present_cpu(index) {
  +   if (!cpu_online(index))
  +   return -EFAULT;
  +   }
 
 EFAULT is not the appropriate error code -- it is for when userspace
 passes a bad virtual address.
 
 Better, don't fail here -- bring the other core out of the low power
 state in order to do the jog.  cpufreq shouldn't stop working just
 because we took a core offline.
 
 What prevents a core from going offline just after you check here?
 
  +   in_jog_process = -1;
  +   mb();
  +   smp_call_function(keep_waking_up, NULL, 0);
 
 What does keep waking up mean?  Something like spin_while_jogging
 might be clearer.
 
  +   local_irq_save(flags);
  +   mb();
  +   /* Wait for the other core to wake. */
  +   while (in_jog_process != 1)
  +   mb();
 
 Timeout?  And more unnecessary mb()s.
 
 Might be nice to support more than two cores, even if this code isn't
 currently expected to be used on such hardware (it's just a generic
 hold other cpus loop; might as well make it reusable).  You could do
 this by using an atomic count for other cores to check in and out of the
 spin loop.

This is just for P1022, a dual-core chip. A separate patch will
support multi-core chips, such as P4080, etc.

 
  +   out_be32(guts + POWMGTCSR, POWMGTCSR_JOG_MASK |
 P1022_POWMGTCSR_MSK);
  +
  +   if (!spin_event_timeout(((in_be32(guts + POWMGTCSR) 
  +   POWMGTCSR_JOG_MASK) == 0), 1, 10)) {
  +   pr_err(%s: Fail to switch the core frequency.\n, __func__);
  +   ret = 

RE: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support

2011-11-11 Thread Zhao Chenhui-B35336


 -Original Message-
 From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org]
 Sent: Friday, November 11, 2011 12:23 PM
 To: Zhao Chenhui-B35336
 Cc: linuxppc-dev@lists.ozlabs.org
 Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
 
 On Fri, 2011-11-04 at 20:31 +0800, Zhao Chenhui wrote:
 
   #ifdef CONFIG_SMP
   /* When we get here, r24 needs to hold the CPU # */
  .globl __secondary_start
  diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
  index 7bf2187..12a54f0 100644
  --- a/arch/powerpc/kernel/smp.c
  +++ b/arch/powerpc/kernel/smp.c
  @@ -381,8 +381,14 @@ void generic_cpu_die(unsigned int cpu)
 
  for (i = 0; i  100; i++) {
  smp_rmb();
  -   if (per_cpu(cpu_state, cpu) == CPU_DEAD)
  +   if (per_cpu(cpu_state, cpu) == CPU_DEAD) {
  +   /*
  +* After another core sets cpu_state to CPU_DEAD,
  +* it needs some time to die.
  +*/
  +   msleep(10);
  return;
  +   }
  msleep(100);
  }
  printk(KERN_ERR CPU%d didn't die...\n, cpu);
 
 I don't really see why you need to wait. This loop is about waiting for
 the CPU to mark itself DEAD, not necessarily to be physically powered
 off.
 

If I don't add this delay, the kernel hangs sometimes when hotpluging a core.
I will move this function to 85xx/smp.c in the next revision.

  +struct epapr_entry {
  +   u32 addr_h;
  +   u32 addr_l;
  +   u32 r3_h;
  +   u32 r3_l;
  +   u32 reserved;
  +   u32 pir;
  +   u32 r6_h;
  +   u32 r6_l;
  +};
 
 This should probably be in a generic location.
 
  +static int is_corenet;
 
 This global is a bit gross...
 
  ...

Agree. I will remove it.

 
  +
  +static void __cpuinit smp_85xx_reset_core(int nr)
  +{
  +   __iomem u32 *vaddr, *pir_vaddr;
  +   u32 val, cpu_mask;
  +
  +   /* If CoreNet platform, use BRR as release register. */
  +   if (is_corenet) {
  +   cpu_mask = 1  nr;
  +   vaddr = ioremap(get_immrbase() + MPC85xx_BRR_OFF, 4);
  +   } else {
  +   cpu_mask = 1  (24 + nr);
  +   vaddr = ioremap(get_immrbase() + MPC85xx_ECM_EEBPCR_OFF, 4);
  +   }
 
 Instead, can't you instead have two functions using a common helper and
 pick/hook the right one ?
 
 Also in what context is that reset_core() called ? Doing ioremap isn't
 something you can do at any random time...
 
 Cheers,
 Ben.
 
 

Thanks. I will fix them.

-chenhui

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev