RE: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch
-Original Message-
From: linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org] On
Behalf Of Scott Wood
Sent: Saturday, November 05, 2011 1:34 AM
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 1/7] powerpc/85xx: re-enable timebase sync disabled by
KEXEC patch
On 11/04/2011 07:29 AM, Zhao Chenhui wrote:
From: Li Yang le...@freescale.com
The timebase sync is not only necessary when using KEXEC. It should also
be used by normal boot up and cpu hotplug. Remove the ifdef added by
the KEXEC patch.
The KEXEC patch didn't just add the ifdef, it also added the initializers:
Yes. But the code suggests that the timebase synchronization is only necessary
for KEXEC, but it turns out that sleep/wakeup also need it. Maybe the
description of the patch need to be changed as KEXEC is not to be blamed.
@@ -105,8 +107,64 @@ smp_85xx_setup_cpu(int cpu_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
};
U-Boot synchronizes the timebase on 85xx. With what chip and U-Boot
version are you seeing this not happen?
I'm curious why don't we make it happen in kernel as we are against adding
dependency to the bootloader? Other architectures don't have this dependency,
it will be better if we don't add this dependency either IMO.
If you are seeing only a small (around one tick) difference, make sure
you're running a U-Boot that has this commit:
commit 7afc45ad7d9493208d89072cbb78a5bfc8034b59
Author: Kumar Gala ga...@kernel.crashing.org
Date: Sun Mar 13 10:55:53 2011 -0500
powerpc/85xx: Fix synchronization of timebase on MP boot
There is a small ordering issue in the master core in that we need
to
make sure the disabling of the timebase in the SoC is visible before
we
set the value to 0. We can simply just read back the value to
synchronizatize the write, before we set TB to 0.
Reported-by: Dan Hettena
Tested-by: Dan Hettena
Signed-off-by: Kumar Gala ga...@kernel.crashing.org
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
On 11/04/2011 07:31 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 e500 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.
Note that there is no guarantee that the bootloader can handle you
resetting a core. In ePAPR the spin table is a one-time release
mechanism, not a core reset mechanism. If this has a U-Boot dependency,
document that.
Actually we suggested to add a spin table in kernel so that we won't have the
dependency on u-boot. But there seems to be some opposite opinion in the
internal discussion. I personally prefer to remove the u-boot dependency and
add the mechanism in kernel.
#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);
It would be better to do this as a call into platform-specific code than
can check registers to determine whether the core has checked out (in
our case, whether it has entered nap) -- or to do a suitable delay for
that platform if this isn't possible.
It will be easier if we can add a platform specific cpu_die instead of using
the generic one?
diff --git a/arch/powerpc/platforms/85xx/smp.c
b/arch/powerpc/platforms/85xx/smp.c
index 9b0de9c..5a54fc1 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -17,6 +17,7 @@
#include linux/of.h
#include linux/kexec.h
#include linux/highmem.h
+#include linux/cpu.h
#include asm/machdep.h
#include asm/pgtable.h
@@ -30,26 +31,141 @@
extern void __early_start(void);
-#define BOOT_ENTRY_ADDR_UPPER 0
-#define BOOT_ENTRY_ADDR_LOWER 1
-#define BOOT_ENTRY_R3_UPPER 2
-#define BOOT_ENTRY_R3_LOWER 3
-#define BOOT_ENTRY_RESV 4
-#define BOOT_ENTRY_PIR 5
-#define BOOT_ENTRY_R6_UPPER 6
-#define BOOT_ENTRY_R6_LOWER 7
-#define NUM_BOOT_ENTRY 8
-#define SIZE_BOOT_ENTRY (NUM_BOOT_ENTRY * sizeof(u32))
-
-static int __init
-smp_85xx_kick_cpu(int nr)
+#define MPC85xx_BPTR_OFF0x00020
+#define MPC85xx_BPTR_EN 0x8000
+#define MPC85xx_BPTR_BOOT_PAGE_MASK 0x00ff
+#define MPC85xx_BRR_OFF 0xe0e4
+#define MPC85xx_ECM_EEBPCR_OFF 0x01010
+#define MPC85xx_PIC_PIR_OFF 0x41090
+
+struct epapr_entry {
ePAPR is more than just the spin table. Call it something like
epapr_spin_table.
+u32 addr_h;
+u32 addr_l;
+u32 r3_h;
+u32 r3_l;
+u32 reserved;
+u32 pir;
+u32 r6_h;
+u32 r6_l;
+};
Get rid of r6, it is not part of the ePAPR spin table.
Agree.
+static int is_corenet;
+static void __cpuinit smp_85xx_setup_cpu(int cpu_nr);
+
+#if defined(CONFIG_HOTPLUG_CPU) defined(CONFIG_PPC32)
Why PPC32?
Not sure if it is the same for e5500. E5500 support will be verified later.
+extern void flush_disable_L1(void);
If this isn't already in a header file, put it in one.
+static void __cpuinit smp_85xx_mach_cpu_die(void)
+{
+unsigned int cpu = smp_processor_id();
+register u32 tmp;
+
+local_irq_disable();
+idle_task_exit();
+generic_set_cpu_dead(cpu);
+smp_wmb();
+
+mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
+mtspr(SPRN_TCR, 0);
If clearing TSR matters at all (I'm not sure that it does), first clear
TCR, then TSR.
+flush_disable_L1();
You'll also need to take down L2 on e500mc.
Right. E500mc support is beyond this patch series. We will work on it after
the e500v2 support is finalized.
+tmp = 0;
+if (cpu_has_feature(CPU_FTR_CAN_NAP))
+tmp = HID0_NAP;
+else if (cpu_has_feature(CPU_FTR_CAN_DOZE))
+tmp = HID0_DOZE;
Those FTR bits are for what we can do in idle, and can be cleared if the
user sets CONFIG_BDI_SWITCH.
It is powersave_nap variable shows what we can do in idle. I think it's
correct to use the CPU_FTR_CAN_* macro as CPU capability.
On 85xx we always want to nap here, and at least on e500mc
RE: [PATCH 3/7] powerpc/85xx: add sleep and deep sleep support
To: Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 3/7] powerpc/85xx: add sleep and deep sleep support Hi Zhao, From: Li Yang leoli at freescale.com Some Freescale chips like MPC8536 and P1022 has deep sleep PM mode in addtion to the sleep PM mode. In sleep PM mode, the clocks of e500 core and unused IP blocks is turned off. IP blocks which are allowed to wake up the processor are still running While in deep sleep PM mode, additionally, the power supply is removed from e500 core and most IP blocks. Only the blocks needed to wake up the chip out of deep sleep are ON. This patch supports 32-bit and 36-bit address space. The deep sleep mode is equal to the Suspend-to-RAM state of Linux Power Management. Command to enter deep sleep mode. echo mem /sys/power/state Thanks a lot for bringing this code to mainline. I was recently involved in enabling deep sleep on a custom P1022 board, and would like to make some remarks based on this experience. 1. I think 85xx deep sleep code would be more complete if you also port FSL BSP code that saves eLBC configuration before entering deep sleep and restores it afterwards. Otherwise all eLBC customization done by u- boot is lost. Thanks for the comment. That work is also being considered for upstream, but not in this series. 2. You should implement fsl_deep_sleep() routine for 85xx. The default implementation in include/linux/fsl_devices.h always returns 0. The routine is used by FSL USB host driver, drivers/usb/host/ehci-fsl.c to restore USB hardware state after deep sleep. With default implementation USB is dead on 85xx after deep sleep if USB PHY is powered down completely. Added to the to-do list. Thanks. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/7] powerpc/85xx: add sleep and deep sleep support
Subject: Re: [PATCH 3/7] powerpc/85xx: add sleep and deep sleep support
On 11/04/2011 07:33 AM, Zhao Chenhui wrote:
+/* Cast the ccsrbar to 64-bit parameter so that the assembly
+ * code can be compatible with both 32-bit 36-bit */
+extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
/*
* Please use proper
* Linux multi-line comment format.
*/
static int pmc_suspend_enter(suspend_state_t state)
{
int ret;
+u32 powmgtreq = 0x0050;
Where does this 0x0050 come from? Please symbolically define
individual bits.
The comment in the asm code says it should be 0x0010, BTW.
I think we should get rid of the powmgtreq argument, as we don't support
multiple modes for mpc85xx_enter_deep_sleep() now.
+
+switch (state) {
+case PM_SUSPEND_MEM:
+#ifdef CONFIG_SPE
+enable_kernel_spe();
+#endif
Should comment that currently only e500v2 hardware supports deep sleep
-- else we'd need to save normal FP here.
+pr_debug(Entering deep sleep\n);
+
+local_irq_disable();
+mpc85xx_enter_deep_sleep(get_immrbase(),
+powmgtreq);
+pr_debug(Resumed from deep sleep\n);
+
+return 0;
+
+/* else fall-through */
+case PM_SUSPEND_STANDBY:
What fall-through? You just returned.
+}
-/* Upon resume, wait for SLP bit to be clear. */
-ret = spin_event_timeout((in_be32(pmc_regs-pmcsr) PMCSR_SLP) ==
0,
- 1, 10) ? 0 : -ETIMEDOUT;
-if (ret)
-dev_err(pmc_dev, tired waiting for SLP bit to clear\n);
-return ret;
}
Remove that blank line as well.
@@ -58,13 +101,23 @@ static const struct platform_suspend_ops
pmc_suspend_ops = {
.enter = pmc_suspend_enter,
};
-static int pmc_probe(struct platform_device *ofdev)
+static int pmc_probe(struct platform_device *pdev)
{
-pmc_regs = of_iomap(ofdev-dev.of_node, 0);
+struct device_node *np = pdev-dev.of_node;
+
+pmc_regs = of_iomap(pdev-dev.of_node, 0);
if (!pmc_regs)
return -ENOMEM;
-pmc_dev = ofdev-dev;
+has_deep_sleep = 0;
+if (of_device_is_compatible(np, fsl,mpc8536-pmc))
+has_deep_sleep = 1;
+
+has_lossless = 0;
+if (of_device_is_compatible(np, fsl,p1022-pmc))
+has_lossless = 1;
+
You never use has_lossless.
It will be used in the later patch in the series.
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 5/7] fsl_pmc: update device bindings
-Original Message- From: linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org [mailto:linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org] On Behalf Of Scott Wood Sent: Saturday, November 05, 2011 4:05 AM To: Zhao Chenhui-B35336 Cc: linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 5/7] fsl_pmc: update device bindings On 11/04/2011 07:36 AM, Zhao Chenhui wrote: From: Li Yang le...@freescale.com Signed-off-by: Li Yang le...@freescale.com --- .../devicetree/bindings/powerpc/fsl/pmc.txt| 63 +++-- -- 1 files changed, 36 insertions(+), 27 deletions(-) diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt index 07256b7..d84b4f8 100644 --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt @@ -9,22 +9,27 @@ Properties: fsl,mpc8548-pmc should be listed for any chip whose PMC is compatible. fsl,mpc8536-pmc should also be listed for any chip - whose PMC is compatible, and implies deep-sleep capability. + whose PMC is compatible, and implies deep-sleep capability and + wake on user defined packet(wakeup on ARP). Why does the PMC care? This is an ethernet controller feature, the PMC is just keeping the wakeup-relevant parts of the ethernet controller alive (whatever they happen to be). Do we have any chips that have ethernet controller support for wake on user-defined packet, but a sleep mode that doesn't let it be used? I think it is more a PMC feature cause Ethernet controller on many SoCs have the filer feature, but only very limited SoCs can support using it as a wake-up source. The differences should be mostly in the PM controller block. Also the wake on user-defined packet feature was never mentioned in the Ethernet controller part of UM. BTW, please remove fsl,mpc8536-pmc from the p1023rds device tree -- it was wrong before (no deep sleep, though it does appear to have jog mode...), and is even more wrong with this provision (it has a different ethernet controller). + fsl,p1022-pmc should be listed for any chip whose PMC is + compatible, and implies lossless Ethernet capability during sleep. fsl,mpc8641d-pmc should be listed for any chip whose PMC is compatible; all statements below that apply to fsl,mpc8548-pmc also apply to fsl,mpc8641d-pmc. Compatibility does not include bit assignments in SCCR/PMCDR/DEVDISR; these - bit assignments are indicated via the sleep specifier in each device's - sleep property. + bit assignments are indicated via the clock nodes. Device which has a + controllable clock source should have a clk-handle property pointing + to the clock node. Do we have any code to use this? Yes, in another patch in the series. Normally that shouldn't matter, but we already an unused binding for this. :-) Please provide rationale for doing it this way. Ideally it should probably use whatever http://devicetree.org/ClockBindings ends up being. I have looked into that binding. The binding was primarily defined for the Linux clock API which is not same as what we are doing here(set wakeup source). If in the future the clock API also covers wakeup related features, we can change to use it. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 7/7] gianfar: add support for wake-on-packet
-Original Message-
From: linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org] On
Behalf Of Scott Wood
Sent: Saturday, November 05, 2011 5:14 AM
To: Zhao Chenhui-B35336
Cc: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Fleming Andy-
AFLEMING
Subject: Re: [PATCH 7/7] gianfar: add support for wake-on-packet
On 11/04/2011 04:11 PM, Scott Wood wrote:
On 11/04/2011 07:40 AM, Zhao Chenhui wrote:
static int gfar_suspend(struct device *dev) { @@ -1268,9 +1443,17
@@ static int gfar_suspend(struct device *dev)
struct gfar __iomem *regs = priv-gfargrp[0].regs;
unsigned long flags;
u32 tempval;
-
int magic_packet = priv-wol_en
- (priv-device_flags FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
+ (priv-wol_opts GIANFAR_WOL_MAGIC);
+ int arp_packet = priv-wol_en
+ (priv-wol_opts GIANFAR_WOL_ARP);
+
+ if (arp_packet) {
+ pmc_enable_wake(priv-ofdev, PM_SUSPEND_MEM, 1);
+ pmc_enable_lossless(1);
+ gfar_arp_suspend(ndev);
+ return 0;
+ }
How do we know this isn't standby?
Or suspend to disk, for that matter?
There is nothing we can do for hibernation. The whole system is literally off.
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 6/7] fsl_pmc: Add API to enable device as wakeup event source
-Original Message-
From: linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org] On
Behalf Of Scott Wood
Sent: Saturday, November 05, 2011 5:14 AM
To: Zhao Chenhui-B35336
Cc: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 6/7] fsl_pmc: Add API to enable device as wakeup event
source
On 11/04/2011 07:39 AM, Zhao Chenhui wrote:
@@ -45,6 +46,72 @@ static int has_lossless;
* code can be compatible with both 32-bit 36-bit */ extern void
mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);
+#ifdef CONFIG_FSL_PMC
+/**
+ * pmc_enable_wake - enable OF device as wakeup event source
+ * @pdev: platform device affected
+ * @state: PM state from which device will issue wakeup events
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the
+system
+ * Error code depending on the platform is returned if both the
+platform and
+ * the native mechanism fail to enable the generation of wake-up
+events */ int pmc_enable_wake(struct platform_device *pdev,
+suspend_state_t state, bool enable)
pmc is too generic for a global function. If this can be either enable
or disable, perhaps it should be something like mpc85xx_pmc_set_wake().
+{
+int ret = 0;
+struct device_node *clk_np;
+u32 *pmcdr_mask;
+
+if (!pmc_regs) {
+printk(KERN_WARNING PMC is unavailable\n);
+return -ENOMEM;
+}
-ENOMEM is not appropriate here, maybe -ENODEV?
Should print __func__ so the user knows what's complaining.
+if (enable !device_may_wakeup(pdev-dev))
+return -EINVAL;
+
+clk_np = of_parse_phandle(pdev-dev.of_node, clk-handle, 0);
+if (!clk_np)
+return -EINVAL;
+
+pmcdr_mask = (u32 *)of_get_property(clk_np, fsl,pmcdr-mask, NULL);
+if (!pmcdr_mask) {
+ret = -EINVAL;
+goto out;
+}
+
+/* clear to enable clock in low power mode */
+if (enable)
+clrbits32(pmc_regs-pmcdr, *pmcdr_mask);
+else
+setbits32(pmc_regs-pmcdr, *pmcdr_mask);
We should probably initialize PMCDR to all bits set (or at least all ones
we know are valid) -- the default should be not a wakeup source.
Ideally I agree with you. But currently we only have the UI of changing
wake-up source for Ethernet device. Will do when we can change all of the
devices.
+/**
+ * pmc_enable_lossless - enable lossless ethernet in low power mode
+ * @enable: True to enable event generation; false to disable */
+void pmc_enable_lossless(int enable) {
+if (enable has_lossless)
+setbits32(pmc_regs-pmcsr, PMCSR_LOSSLESS);
+else
+clrbits32(pmc_regs-pmcsr, PMCSR_LOSSLESS); }
+EXPORT_SYMBOL_GPL(pmc_enable_lossless);
+#endif
Won't we overwrite this later?
You are right. Will remove the code that overwrite this.
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 7/7] gianfar: add support for wake-on-packet
-Original Message-
From: linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=freescale@lists.ozlabs.org] On
Behalf Of Scott Wood
Sent: Saturday, November 05, 2011 5:12 AM
To: Zhao Chenhui-B35336
Cc: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Fleming Andy-
AFLEMING
Subject: Re: [PATCH 7/7] gianfar: add support for wake-on-packet
On 11/04/2011 07:40 AM, Zhao Chenhui wrote:
diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
index 2c6be03..543e36c 100644
--- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
+++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
@@ -56,6 +56,9 @@ Properties:
hardware.
- fsl,magic-packet : If present, indicates that the hardware supports
waking up via magic packet.
+ - fsl,wake-on-filer : If present, indicates that the hardware
supports
+waking up via arp request to local ip address or unicast packet to
+local mac address.
Is there any way to determine this at runtime via the device's registers?
I think TSEC_ID2[TSEC_CFG] can be used. The manual describes it
awkwardly, but it looks like 0x20 is the bit for the filer.
That bit only defines the filer feature but not wakeup on it. Another solution
is to get the capability from the fsl_pmc driver, but will make the driver a
lot more complex.
@@ -751,7 +764,6 @@ static int gfar_of_init(struct platform_device
*ofdev, struct net_device **pdev)
FSL_GIANFAR_DEV_HAS_PADDING |
FSL_GIANFAR_DEV_HAS_CSUM |
FSL_GIANFAR_DEV_HAS_VLAN |
-FSL_GIANFAR_DEV_HAS_MAGIC_PACKET |
FSL_GIANFAR_DEV_HAS_EXTENDED_HASH |
FSL_GIANFAR_DEV_HAS_TIMER;
This is an unrelated change. Are there any eTSECs that don't support
magic packet?
I'm not sure. But as we have the property for it in device tree, we use it.
+static int gfar_get_ip(struct net_device *dev)
+{
+struct gfar_private *priv = netdev_priv(dev);
+struct in_device *in_dev = (struct in_device *)dev-ip_ptr;
+struct in_ifaddr *ifa;
+
+if (in_dev != NULL) {
+ifa = (struct in_ifaddr *)in_dev-ifa_list;
+if (ifa != NULL) {
+memcpy(priv-ip_addr, ifa-ifa_address, 4);
+return 0;
+}
+}
+return -ENOENT;
+}
Unnecessary cast, ifa_list is already struct in_ifaddr *.
Better, use for_primary_ifa(), and document that you won't wake on ARP
packets for secondary IP addresses.
static int gfar_suspend(struct device *dev)
{
@@ -1268,9 +1443,17 @@ static int gfar_suspend(struct device *dev)
struct gfar __iomem *regs = priv-gfargrp[0].regs;
unsigned long flags;
u32 tempval;
-
int magic_packet = priv-wol_en
-(priv-device_flags FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
+(priv-wol_opts GIANFAR_WOL_MAGIC);
+int arp_packet = priv-wol_en
+(priv-wol_opts GIANFAR_WOL_ARP);
+
+if (arp_packet) {
+pmc_enable_wake(priv-ofdev, PM_SUSPEND_MEM, 1);
+pmc_enable_lossless(1);
+gfar_arp_suspend(ndev);
+return 0;
+}
How do we know this isn't standby?
Maybe we can remove the PM state parameter from the API.
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
-Original Message-
From: Wood Scott-B07421
Sent: Wednesday, November 09, 2011 4:58 AM
To: Li Yang-R58472
Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
On 11/08/2011 04:05 AM, Li Yang-R58472 wrote:
Subject: Re: [PATCH 2/7] powerpc/85xx: add HOTPLUG_CPU support
On 11/04/2011 07:31 AM, Zhao Chenhui wrote:
+static int is_corenet;
+static void __cpuinit smp_85xx_setup_cpu(int cpu_nr);
+
+#if defined(CONFIG_HOTPLUG_CPU) defined(CONFIG_PPC32)
Why PPC32?
Not sure if it is the same for e5500. E5500 support will be verified
later.
It's better to have 64-bit silently do nothing here?
+ flush_disable_L1();
You'll also need to take down L2 on e500mc.
Right. E500mc support is beyond this patch series. We will work on it
after the e500v2 support is finalized.
I saw some support with is_corenet... If we don't support e500mc, make
sure the code doesn't try to run on e500mc.
The is_corenet code is just a start of the e500mc support and is far from
complete.
This isn't an e500v2-only BSP you're putting the code into. :-)
Yes, I know. But it will take quite some time to perfect the support for
different type of cores. I'd like to make the effort into stages. We want to
push the e500v2 support in first and add support to newer cores later so that
we don't need to re-spin the patches again and again. And the upstream kernel
can get the PM functionality at least for e500v2 earlier. Anyway, we need to
update the title of the patch to be more specific on e500v2.
+ tmp = 0;
+ if (cpu_has_feature(CPU_FTR_CAN_NAP))
+ tmp = HID0_NAP;
+ else if (cpu_has_feature(CPU_FTR_CAN_DOZE))
+ tmp = HID0_DOZE;
Those FTR bits are for what we can do in idle, and can be cleared if
the user sets CONFIG_BDI_SWITCH.
It is powersave_nap variable shows what we can do in idle.
No, that shows what the user wants to do in idle.
I think it's correct to use the CPU_FTR_CAN_* macro as CPU capability.
This is 85xx-specific code. We always can, and want to, nap here (though
the way we enter nap will be different on e500mc and up) -- even if it's
broken to use it for idle (such as on p4080, which would miss doorbells as
wake events).
Ok. Will stick to Nap.
+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);
+ }
Please use the device tree node, not get_immrbase().
The get_immrbase() implementation uses the device tree node.
I mean the guts node. get_immrbase() should be avoided where possible.
Ok. I got your point to use offset from guts.
Also, some people might care about latency to enter/exit deep sleep.
Searching through the device tree at this point (rather than on init)
slows that down.
Actually the get_immrbase() is smarter than you thought. :) It only search the
device tree on first call and returns the saved value on follow up calls.
+static int __cpuinit smp_85xx_map_bootpg(u32 page) {
+ __iomem u32 *bootpg_ptr;
+ u32 bptr;
+
+ /* Get the BPTR */
+ bootpg_ptr = ioremap(get_immrbase() + MPC85xx_BPTR_OFF, 4);
+
+ /* Set the BPTR to the secondary boot page */
+ bptr = MPC85xx_BPTR_EN | (page MPC85xx_BPTR_BOOT_PAGE_MASK);
+ out_be32(bootpg_ptr, bptr);
+
+ iounmap(bootpg_ptr);
+ return 0;
+}
Shouldn't the boot page already be set by U-Boot?
The register should be lost after a deep sleep. Better to do it again.
How can it be lost after a deep sleep if we're relying on it to hold our
wakeup code?
In order to wake up from deep sleep, the boot page has been set to the deep
sleep restoration function. We need to set it back to the bootpage from u-boot.
It's not better to do it again if we're making a bad assumption about
the code and the table being in the same page.
Currently we are reusing the whole boot page including the spin-table from
u-boot. Are you suggesting to move just the boot page to kernel?
local_irq_save(flags);
- out_be32(bptr_vaddr + BOOT_ENTRY_PIR, nr);
+ out_be32(epapr-pir, hw_cpu);
#ifdef CONFIG_PPC32
- out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
+#ifdef CONFIG_HOTPLUG_CPU
+ if (system_state == SYSTEM_RUNNING) {
+ out_be32(epapr-addr_l, 0);
+ smp_85xx_map_bootpg((u32)(*cpu_rel_addr PAGE_SHIFT));
Why is this inside PPC32?
Not tested on 64-bit yet. Might require a different implementation on
PPC64.
Please make a reasonable effort to do things in a way that works on both.
It shouldn't be a big deal to test that it doesn't break booting on p5020.
Will do. But in stages
RE: [PATCH 5/7] fsl_pmc: update device bindings
-Original Message- From: Wood Scott-B07421 Sent: Wednesday, November 09, 2011 4:40 AM To: Li Yang-R58472 Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 5/7] fsl_pmc: update device bindings On 11/08/2011 04:56 AM, Li Yang-R58472 wrote: diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt index 07256b7..d84b4f8 100644 --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt @@ -9,22 +9,27 @@ Properties: fsl,mpc8548-pmc should be listed for any chip whose PMC is compatible. fsl,mpc8536-pmc should also be listed for any chip - whose PMC is compatible, and implies deep-sleep capability. + whose PMC is compatible, and implies deep-sleep capability and + wake on user defined packet(wakeup on ARP). Why does the PMC care? This is an ethernet controller feature, the PMC is just keeping the wakeup-relevant parts of the ethernet controller alive (whatever they happen to be). Do we have any chips that have ethernet controller support for wake on user-defined packet, but a sleep mode that doesn't let it be used? I think it is more a PMC feature cause Ethernet controller on many SoCs have the filer feature, but only very limited SoCs can support using it as a wake-up source. The differences should be mostly in the PM controller block. Have you tried waking from sleep with it on those other SoCs? We have tried and it's not working. Although the filer is an eTSEC feature, waking up on it is really a complex system wide change including eTSEC, DDR, ECM, PIC, and etc. And currently it's tied up to deep sleep feature. So I would like it to be part of the SoC integration domain i.e. PMC. Also the wake on user-defined packet feature was never mentioned in the Ethernet controller part of UM. AFAICT all this feature is, is programming the Ethernet controller to filter out some packets that we don't want to wake us up, and then refraining from entering magic packet mode. What PMC registers are programmed any differently for this? It's in the PM part of the manual because that's where they generally describe issues related to PM. They describe magic packet there too. The set of devices which are still active during sleep is a different issue from the conditions on which a device will request a wake. I also don't trust our PM documentation very much. It's ambiguous just what gets shut down in ordinary sleep mode. E.g. the mpc8544 manual talks about external interrupts, but in one place it looks like it means external to the SoC: In sleep mode, all clocks internal to the e500 core are turned off, including the timer facilities clock. All I/O interfaces in the device logic are also shut down. Only the clocks to the MPC8544E PIC are still running so that an external interrupt can wake up the device. But the note immediately below that seems to imply they're talking about external to the core: Only external interrupts can wake the MPC8544E from sleep mode. Internal interrupt sources like the core interval timer or watchdog timer depend on an active clock for their operation and these are disabled in sleep mode. Normally that shouldn't matter, but we already an unused binding for this. :-) Please provide rationale for doing it this way. Ideally it should probably use whatever http://devicetree.org/ClockBindings ends up being. I have looked into that binding. The binding was primarily defined for the Linux clock API which is not same as what we are doing here(set wakeup source). If in the future the clock API also covers wakeup related features, we can change to use it. While Linux APIs can serve as an inspiration, they're not the basis of device tree bindings. The device tree is not Linux-specific, and should not change just because Linux changes, but rather should describe the hardware. We want to get this right the first time. What we are describing here is how a device's clock is connected to the PMC. AFAIKT, the purpose of defining the clock binding is used to describe the topology of clocks in a system. And then used for runtime control of enabling/disabling clocks or getting/changing clock frequencies. But in this PM case, we just set the wakeup capability and provide little runtime control of the clocks for on-chip devices. And it doesn't get any benefit of using this binding. If your concern is the confusion with the already existing clock binding, we can remove the clk in the name of the PM binding. If we are to use the clock binding, I think adding the CSB clock, DDR clock, core clock, and etc with this binding is more appropriate. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v2 2/7] powerpc/85xx: add HOTPLUG_CPU support
Cc: linuxppc-dev@lists.ozlabs.org; Li Yang-R58472
Subject: Re: [PATCH v2 2/7] powerpc/85xx: add HOTPLUG_CPU support
On 11/16/2011 03:55 AM, Zhao Chenhui wrote:
+static void __cpuinit smp_85xx_mach_cpu_die(void) {
+unsigned int cpu = smp_processor_id();
+register u32 tmp;
+
+local_irq_disable();
+idle_task_exit();
+generic_set_cpu_dead(cpu);
+mb();
+
+mtspr(SPRN_TCR, 0);
+mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
Clearing these bits in TSR should be unnecessary since we clear TCR -- and
doesn't really accomplish anything since the TSR bits can continue to be
set.
I also recommend setting the CORE_IRQ_MASK and CORE_CI_MASK in the POWMGTCSR
register so that no interrupt will wakeup the core from NAP.
If watchdog is in use, we need to set the period to the highest possible
to effectively disable it.
Setting it to the highest timeout doesn't really disable the watchdog. The
best way for disabling the wdt is to reset the core, although it's a bit too
complex to do.
+if (cpu_has_feature(CPU_FTR_CAN_NAP)) {
Again, don't check this. On 85xx, we *always* can and should use nap here.
At best this is noise, at worst this will cause problems if
CONFIG_BDI_SWITCH is enabled, or if CPU_FTR_CAN_NAP is cleared for any
other reason (e.g. it's not set on e500mc, and the reason isn't that the
nap implementation is different (which it is), but that it's not usable in
the idle loop).
+static int __cpuinit smp_85xx_kick_cpu(int nr)
+
{
unsigned long flags;
const u64 *cpu_rel_addr;
-__iomem u32 *bptr_vaddr;
+__iomem struct epapr_spin_table *epapr;
Please don't call this just epapr. That's like calling a reference to
any powerpc-specific struct powerpc.
How about spin_table?
You mean the name of the variable not the structure, right? I agree.
-out_be32(bptr_vaddr + BOOT_ENTRY_PIR, hw_cpu);
+out_be32(epapr-pir, hw_cpu);
#ifdef CONFIG_PPC32
-out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
+#ifdef CONFIG_HOTPLUG_CPU
+/* Corresponding to generic_set_cpu_dead() */
+generic_set_cpu_up(nr);
+
+if (system_state == SYSTEM_RUNNING) {
+out_be32(epapr-addr_l, 0);
+
+smp_85xx_set_bootpg((u32)(*cpu_rel_addr PAGE_SHIFT));
As previously requested, please document why you're setting the boot page
here. This should really be done when you resume from deep sleep, rather
than here, and should be a restoration of the value that the register held
prior to deep sleep.
struct smp_ops_t smp_85xx_ops = {
.kick_cpu = smp_85xx_kick_cpu,
+.setup_cpu = smp_85xx_setup_cpu,
+#ifdef CONFIG_HOTPLUG_CPU
+.cpu_disable= generic_cpu_disable,
+.cpu_die= generic_cpu_die,
+#endif
Only fill these fields in on e500v1/v2, until we properly support e500mc.
Likewise in ppc_md.cpu_die and anywhere else we advertise this
functionality.
Is there a standard function call that can tell that it is an e500mc not legacy
e500?
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v2] USB: fsl_udc_core: use usb_endpoint_xfer_isoc to judge ISO XFER
Subject: [PATCH v2] USB: fsl_udc_core: use usb_endpoint_xfer_isoc to judge ISO XFER Some ISO gadgets, like audio, has SYNC attribute as well as USB_ENDPOINT_XFER_ISOC for their bmAttributes at ISO endpoint descriptor. So, it needs to use usb_endpoint_xfer_isoc to judge ISO XFER. Signed-off-by: Peter Chen peter.c...@freescale.com Acked-by: Li Yang le...@freescale.com - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v2 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch
Subject: Re: [PATCH v2 1/7] powerpc/85xx: re-enable timebase sync disabled by KEXEC patch On Fri, Nov 18, 2011 at 08:35:02AM -0600, Kumar Gala wrote: On Nov 16, 2011, at 12:42 PM, Scott Wood wrote: On 11/16/2011 03:55 AM, Zhao Chenhui wrote: From: Li Yang le...@freescale.com The timebase sync is not only necessary when using KEXEC. It should also be used by normal boot up and cpu hotplug. Remove the ifdef added by the KEXEC patch. Again, no it should not be used by normal boot up (whether KEXEC support is enabled or not). We should only do timebase sync when we actually need to (when we've actually just reset a core), and we should do it the way U-Boot does rather than with smp-tbsync.c. While looking into the timebase sync code in u-boot, I have a few questions. /* enable time base at the platform */ if (whoami) devdisr |= MPC85xx_DEVDISR_TB1; else devdisr |= MPC85xx_DEVDISR_TB0; out_be32(gur-devdisr, devdisr); /* readback to sync write */ in_be32(gur-devdisr); mtspr(SPRN_TBWU, 0); mtspr(SPRN_TBWL, 0); What are the TBWU/TBWL registers? I can't find the definition of them in either e500 RM or booke RM. Are they valid to be used? What is the result of writing to them? Aren't the SPR registers core specific? How can we set the TB for the other cores? devdisr = ~(MPC85xx_DEVDISR_TB0 | MPC85xx_DEVDISR_TB1); out_be32(gur-devdisr, devdisr); Also in the UM, I read Blocks disabled by DEVDISR must not be re-enabled without a hard reset. Is it safe we enable it here? - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] usb/fsl_udc: fix dequeuing a request in progress
Subject: Re: [PATCH] usb/fsl_udc: fix dequeuing a request in progress
On Fri, Nov 11, 2011 at 08:38:13PM +0800, Li Yang wrote:
The original implementation of dequeuing a request in progress is not
correct. Change to use a correct process and also clean up the
related functions a little bit.
Signed-off-by: Li Yang le...@freescale.com
---
drivers/usb/gadget/fsl_udc_core.c | 62 +-
--
1 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/gadget/fsl_udc_core.c
b/drivers/usb/gadget/fsl_udc_core.c
index b2c44e1..beef9b7 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -696,12 +696,31 @@ static void fsl_free_request(struct usb_ep *_ep,
struct usb_request *_req)
kfree(req);
}
-/*---
--*/
+/* Actually add a dTD chain to an empty dQH and let go */ static void
+fsl_prime_ep(struct fsl_ep *ep, struct ep_td_struct *td) {
+struct ep_queue_head *qh = ep-qh;
It seems to can't get the correct qh pointer, you may still need to use
below code to get it
int i = ep_index(ep) * 2 + ep_is_in(ep);
struct ep_queue_head *dQH = ep-udc-ep_qh[i];
Thanks for trying.It will be much easier if we can dereference QH from the
ep structure. It is really strange that the ep-qh pointer is not working
somehow.
We have initialized it in struct_ep_setup():
ep-qh = udc-ep_qh[index];
Can you do me a favor on investigating why it's failing?
Thanks,
Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] usb/fsl_udc: fix dequeuing a request in progress
-Original Message- From: Peter Chen [mailto:hzpeterc...@gmail.com] Sent: Wednesday, November 23, 2011 11:02 AM To: Li Yang-R58472 Cc: Chen Peter-B29397; ba...@ti.com; gre...@suse.de; linux- u...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] usb/fsl_udc: fix dequeuing a request in progress On Tue, Nov 22, 2011 at 7:48 PM, Peter Chen hzpeterc...@gmail.com wrote: It seems to can't get the correct qh pointer, you may still need to use below code to get it int i = ep_index(ep) * 2 + ep_is_in(ep); struct ep_queue_head *dQH = ep-udc-ep_qh[i]; Thanks for trying. It will be much easier if we can dereference QH from the ep structure. It is really strange that the ep-qh pointer is not working somehow. Seems only ep0-out's qh pointer is assigned at ep structure. At probe: /* setup udc-eps[] for ep0 */ struct_ep_setup(udc_controller, 0, ep0, 0); We have initialized it in struct_ep_setup(): ep-qh = udc-ep_qh[index]; Can you do me a favor on investigating why it's failing? Leo, I have debugged this issue at my board just now, the reason of failure is we only have one ep struct for ep0, so when talking about ep0, it always pointers to udc-ep[0]. So even we initialize the current qh address for ep0in at probe, it still can't get the ep0in's qh address through ep structure, as ep0 is always udc-ep[0]. Oops. I forgot the fact that we used a single ep structure to handle both IN and OUT ep0 because the gadget layer only knows about one ep0 structure. I guess currently we have no other way out unless the gadget layer do honor ep0 with direction. IMHO, it is a limitation to current gadget APIs that each udc driver need to take too much care of the protocol related stuff on ep0. Thanks, Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3] mtd/nand : workaround for Freescale FCM to support large-page Nand chip
Subject: [PATCH 3/3] mtd/nand : workaround for Freescale FCM to support
large-page Nand chip
From: Liu Shuo b35...@freescale.com
Freescale FCM controller has a 2K size limitation of buffer RAM. In order
to support the Nand flash chip whose page size is larger than 2K bytes,
we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save
them to a large buffer.
Signed-off-by: Liu Shuo b35...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
---
drivers/mtd/nand/fsl_elbc_nand.c | 211
+++---
1 files changed, 194 insertions(+), 17 deletions(-)
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
b/drivers/mtd/nand/fsl_elbc_nand.c
index d634c5f..c96e714 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
[snip]
+static void io_to_buffer(struct mtd_info *mtd, int subpage, int oob)
+{
+ struct nand_chip *chip = mtd-priv;
+ struct fsl_elbc_mtd *priv = chip-priv;
+ struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv-ctrl-nand;
+ void *src, *dst;
+ int len = (oob ? 64 : 2048);
+
+ if (oob)
+ dst = elbc_fcm_ctrl-buffer + mtd-writesize + subpage * 64;
+ else
+ dst = elbc_fcm_ctrl-buffer + subpage * 2048;
+
+ src = elbc_fcm_ctrl-addr + (oob ? 2048 : 0);
+ memcpy_fromio(dst, src, len);
Might be safer to use _memcpy_fromio()
+}
+
+static void buffer_to_io(struct mtd_info *mtd, int subpage, int oob)
+{
+ struct nand_chip *chip = mtd-priv;
+ struct fsl_elbc_mtd *priv = chip-priv;
+ struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv-ctrl-nand;
+ void *src, *dst;
+ int len = (oob ? 64 : 2048);
+
+ if (oob)
+ src = elbc_fcm_ctrl-buffer + mtd-writesize + subpage * 64;
+ else
+ src = elbc_fcm_ctrl-buffer + subpage * 2048;
+
+ dst = elbc_fcm_ctrl-addr + (oob ? 2048 : 0);
+ memcpy_toio(dst, src, len);
+
+ /* See the in_8() in fsl_elbc_write_buf() */
+ in_8(elbc_fcm_ctrl-addr);
Should be safer to read back the last char.
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3] mtd/nand : workaround for Freescale FCM to support large-page Nand chip
Subject: Re: [PATCH 3/3] mtd/nand : workaround for Freescale FCM to support large-page Nand chip On Thu, 2011-11-24 at 08:41 +0800, b35...@freescale.com wrote: + /* +* Freescale FCM controller has a 2K size limitation of buffer +* RAM, so elbc_fcm_ctrl-buffer have to be used if writesize +* of chip is greater than 2048. +* We malloc a large enough buffer (maximum page size is 16K). +*/ + elbc_fcm_ctrl-buffer = kmalloc(1024 * 16 + 1024, GFP_KERNEL); Are there NANDs with 16KiB page size? We are not sure, but are there possibility that chip with 16K page will appear? Or maybe we can add a MACRO for the maximum page size? - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH][RFC] fsldma: fix performance degradation by optimizing spinlock use.
Subject: Re: [PATCH][RFC] fsldma: fix performance degradation by optimizing spinlock use. On Thu, Nov 24, 2011 at 08:12:25AM +, Shi Xuelin-B29237 wrote: Hi Ira, Thanks for your review. After second thought, I think your scenario may not occur. Because the cookie 20 we query must be returned by fsl_dma_tx_submit(...) in practice. We never query a cookie not returned by fsl_dma_tx_submit(...). I agree about this part. When we call fsl_tx_status(20), the chan-common.cookie is definitely wrote as 20 and cpu2 could not read as 19. This is what I don't agree about. However, I'm not an expert on CPU cache vs. memory accesses in an multi-processor system. The section titled CACHE COHERENCY in Documentation/memory-barriers.txt leads me to believe that the scenario I described is possible. For Freescale PowerPC, the chip automatically takes care of cache coherency. Even if this is a concern, spinlock can't address it. What happens if CPU1's write of chan-common.cookie only goes into CPU1's cache. It never makes it to main memory before CPU2 fetches the old value of 19. I don't think you should see any performance impact from the smp_mb() operation. Smp_mb() do have impact on performance if it's in the hot path. While it might be safer having it, I doubt it is really necessary. If the CPU1 doesn't have the updated last_used, it's shouldn't have known there is a cookie 20 existed either. - Leo Thanks, Ira -Original Message- From: Ira W. Snyder [mailto:i...@ovro.caltech.edu] Sent: 2011年11月23日 2:59 To: Shi Xuelin-B29237 Cc: dan.j.willi...@intel.com; Li Yang-R58472; z...@zh-kernel.org; vinod.k...@intel.com; linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org Subject: Re: [PATCH][RFC] fsldma: fix performance degradation by optimizing spinlock use. On Tue, Nov 22, 2011 at 12:55:05PM +0800, b29...@freescale.com wrote: From: Forrest Shi b29...@freescale.com dma status check function fsl_tx_status is heavily called in a tight loop and the desc lock in fsl_tx_status contended by the dma status update function. this caused the dma performance degrades much. this patch releases the lock in the fsl_tx_status function. I believe it has no neglect impact on the following call of dma_async_is_complete(...). we can see below three conditions will be identified as success a) x complete use b) x complete+N use+N c) x complete use+N here complete is the completed_cookie, use is the last_used cookie, x is the querying cookie, N is MAX cookie when chan-completed_cookie is being read, the last_used may be incresed. Anyway it has no neglect impact on the dma status decision. Signed-off-by: Forrest Shi xuelin@freescale.com --- drivers/dma/fsldma.c |5 - 1 files changed, 0 insertions(+), 5 deletions(-) diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index 8a78154..1dca56f 100644 --- a/drivers/dma/fsldma.c +++ b/drivers/dma/fsldma.c @@ -986,15 +986,10 @@ static enum dma_status fsl_tx_status(struct dma_chan *dchan, struct fsldma_chan *chan = to_fsl_chan(dchan); dma_cookie_t last_complete; dma_cookie_t last_used; - unsigned long flags; - - spin_lock_irqsave(chan-desc_lock, flags); This will cause a bug. See below for a detailed explanation. You need this instead: /* * On an SMP system, we must ensure that this CPU has seen the * memory accesses performed by another CPU under the * chan-desc_lock spinlock. */ smp_mb(); last_complete = chan-completed_cookie; last_used = dchan-cookie; - spin_unlock_irqrestore(chan-desc_lock, flags); - dma_set_tx_state(txstate, last_complete, last_used, 0); return dma_async_is_complete(cookie, last_complete, last_used); } Facts: - dchan-cookie is the same member as chan-common.cookie (same memory location) - chan-common.cookie is the last allocated cookie for a pending transaction - chan-completed_cookie is the last completed transaction I have replaced dchan-cookie with chan-common.cookie in the below explanation, to keep everything referenced from the same structure. Variable usage before your change. Everything is used locked. - RW chan-common.cookie(fsl_dma_tx_submit) - R chan-common.cookie(fsl_tx_status) - R chan-completed_cookie (fsl_tx_status) - W chan-completed_cookie (dma_do_tasklet) Variable usage after your change: - RW chan-common.cookieLOCKED - R chan-common.cookieNO LOCK - R chan-completed_cookie NO LOCK - W chan-completed_cookie LOCKED What if we assume that you have a 2 CPU system (such as a P2020). After your changes, one possible sequence is: === CPU1
RE: [PATCH v2] Integrated Flash Controller support
-Original Message- From: Artem Bityutskiy [mailto:dedeki...@gmail.com] Sent: Wednesday, November 30, 2011 4:51 PM To: Kumar Gala Cc: Wood Scott-B07421; Li Yang-R58472; Liu Shuo-B35362; linux- ker...@vger.kernel.org Kernel; linux-...@lists.infradead.org; Andrew Morton; David Woodhouse; linuxppc-dev@lists.ozlabs.org list Subject: Re: [PATCH v2] Integrated Flash Controller support On Tue, 2011-11-29 at 19:47 -0600, Kumar Gala wrote: As Scott said, I was more asking about the 2nd patch in the sequence which did touch MTD. Since that one is dependent on this patch, wondering how we wanted to handle them. I do not have time to review it, but it looks OK, so I'd suggest to merge it vie the same tree as the rest of the patches. Hi Artem, So what is your suggestion on this patch? Can we regard your previous email as an ACK and merge it through the powerpc tree? Or do you prefer to merge them through the MTD tree with Kumar's ACK instead? - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RFC] Multi queue support in ethernet/freescale/ucc_geth.c
-Original Message- From: Paul Gortmaker [mailto:paul.gortma...@windriver.com] Sent: Friday, February 03, 2012 6:42 AM To: Li Yang-R58472 Cc: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org Subject: [RFC] Multi queue support in ethernet/freescale/ucc_geth.c Hi Li, Hi Paul, Sorry for the late response due to holidays. A while back DaveM mentioned that it would be good to break out the ring allocations[1] in this driver. I was looking at it, and in the process noticed this: $ grep 'numQueues.*=' drivers/net/ethernet/freescale/ucc_geth.c .numQueuesTx = 1, .numQueuesRx = 1, $ My interpretation of the above is that there is no way (aside from a code edit) to enable multi queue support. They are only ever assigned one time, to a value of one. Assuming I'm not missing something obvious, is the multi queue support functional and tested, or just old code that never got tested and subsequently enabled? Previously the device is only used on single core cpu, so we didn't have the incentive to enable multi-queue. It is not tested on Linux currently. The reason I ask, is that the ring allocation code gets rid of the loop wrapping it, if the driver is really only meant to ever have just single queues for Rx/Tx. And other areas of the driver can also be simplified accordingly as well. Well. I would prefer the other way which is to add the multi-queue support as we are using the QE in multi-core SoC and the current driver is having almost all the code needed for multi-queue except interface to the protocol layer. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V2 RESEND] fsl-sata: I/O load balancing
I tried a debian install on the p5020ds here and I find SATA to be extremely slow, generating millions of interrupts. I think the defaults should be a lot more aggressive at coalescing. The P5020 has a hardware problem with SATA, making it generate more interrupts than it should. A new revision of the silicon will fix it. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 1/2] powerpc/85xx: fix problem that prevents PHYS_64BIT from configurable
Subject: Re: [PATCH 1/2] powerpc/85xx: fix problem that prevents PHYS_64BIT from configurable On Feb 16, 2012, at 6:10 AM, Li Yang wrote: Fix the problem that large physical address support cannot be disabled when some platforms which only provides 36-bit support are selected. According to the philosophy of kernel config enabling a platform support doesn't mean the kernel is only running on that platform. Remove the auto selection of PHYS_64BIT option for these platforms. They will need to use a 36bit default config that selects PHYS_64BIT explicitly. The reason why we need to keep PHYS_64BIT option configurable is that enabling it cause negative performance impact on various aspects like TLB miss and physical address manipulating. We should not enable it unless really needed, e.g. use large memory of 4GB or bigger. Signed-off-by: Li Yang le...@freescale.com --- arch/powerpc/platforms/85xx/Kconfig |6 -- 1 files changed, 0 insertions(+), 6 deletions(-) Nak, this isn't correct. For some of these platforms like P2041RDB, P3041DS, P3060QDS, P4080DS, P5020DS only a 36-bit physical address map is supported by u-boot and the device tree. This was a decision that was made to NOT support 32-bit address map for these boards and accept the performance implication of it to reduce the # of builds, etc. Ok. Although personally I don't think # of builds matters that much. Additionally, outside of maybe P2041RDB I believe the majority of these boards ship with 4G of DDR (but that off the top of my head) and thus require the 36-bit / PHYS_64BIT support to be enabled. I know that current support of DPAA boards requires 36-bit. But I don't think they need to select the PHYS_64BIT option directly and make it not configurable. It's conflicting with the logic that enabling a platform support doesn't mean the kernel is only running on that platform. Btw: what's your recommendations on solving this? - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable
Subject: Re: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable Huang Changming-R66093 wrote: I have one similar patch to remove the select PHYS_64BIT. http://patchwork.ozlabs.org/patch/132351/ That one doesn't update the defconfigs, which means that the default kernel will not have PHYS_64BIT enabled. I think it is not necessary to enable the 64BIT, if customer want to enable it, he can do it manually. I agree with Changming that we shouldn't setting PHYS_64BIT by default. For the platforms covered by the mpc85xx_defconfig, most user won't need the PHYS_64BIT. We shouldn't set the one with worse performance and unnecessary to most people as default. Also all these platforms supports 32-bit mode. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable
-Original Message- From: Tabi Timur-B04825 Sent: Friday, February 24, 2012 10:46 AM To: Li Yang-R58472 Cc: Huang Changming-R66093; ga...@kernel.crashing.org; b...@kernel.crashing.org; Wood Scott-B07421; linuxppc-...@ozlabs.org Subject: Re: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable Li Yang-R58472 wrote: I agree with Changming that we shouldn't setting PHYS_64BIT by default. The default kernel should always be the compatible with as much as possible. Disabling PHYS_64BIT by default means that the default kernel will not work with a 36-bit DTS. If you attempt to boot such a kernel with a 36-bit DTS, there will be no text output. Most people will not know why it's not working. So the safest option is for PHYS_64BIT to be enabled by default. That way, the kernel will always work. Even though the user still need to know the addressing mode that u-boot is using. It won't work if the addressing mode of u-boot and device tree are different. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable
Subject: Re: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable Li Yang-R58472 wrote: Even though the user still need to know the addressing mode that u-boot is using. It won't work if the addressing mode of u-boot and device tree are different. U-Boot will tell the user if the DT does not match. I added code to U- Boot to do that. So if you have a 36-bit U-Boot and a 32-bit DT, then you will get a warning. If you have a 36-bit U-boot and a 36-bit DT and a 32-bit kernel, you will get nothing. But if you have a 32-bit U-boot and a 32-bit DT and a 36-bit kernel, then that will work. A 36-bit kernel works with 32-bit *and* 36-bit DTs. This is why a 36-bit kernel should be the default. The mpc85xx_defconfig does include silicons with e500v1 core which doesn't have the 36-bit support. Won't enabling 36-bit support by default break the support for them? - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable
Subject: Re: [PATCH] powerpc/85xx: allow CONFIG_PHYS_64BIT to be selectable Li Yang-R58472 wrote: The mpc85xx_defconfig does include silicons with e500v1 core which doesn't have the 36-bit support. Won't enabling 36-bit support by default break the support for them? No. The kernel will detect at runtime that that it's an e500v1 core and it won't try to create 36-bit TLBs. (e.g. it won't write to MAS7). It's a good point. Why can't we decide to use 32-bit/36-bit TLB at runtime even for e500v2? Please remember that the Kconfig for the P1022DS already forced PHYS_64BIT for all mpc85xx platforms. All we're doing is making it possible to deselect PHYS_64BIT. I think it's a side-effect introduced by P1022DS support and need to be fixed. There was no mentioning of enforcing 36-bit for all mpc85xx platforms. - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
