* [PATCH v2 2/2] ppc: bpf_jit: support MOD operation
From: Vladimir Murzin @ 2013-09-28 8:22 UTC (permalink / raw)
To: linuxppc-dev
Cc: matt, netdev, Vladimir Murzin, dborkman, edumazet, paulus, davem
In-Reply-To: <1380356521-3432-1-git-send-email-murzin.v@gmail.com>
commit b6069a9570 (filter: add MOD operation) added generic
support for modulus operation in BPF.
This patch brings JIT support for PPC64
Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
Acked-by: Matt Evans <matt@ozlabs.org>
---
Changelog
v1->v2
Definition for r_scratch2 was moved to header file.
arch/powerpc/net/bpf_jit.h | 1 +
arch/powerpc/net/bpf_jit_comp.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 8a5dfaf..42a115a 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -39,6 +39,7 @@
#define r_X 5
#define r_addr 6
#define r_scratch1 7
+#define r_scratch2 8
#define r_D 14
#define r_HL 15
#define r_M 16
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..cbb2702 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -193,6 +193,26 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image,
PPC_MUL(r_A, r_A, r_scratch1);
}
break;
+ case BPF_S_ALU_MOD_X: /* A %= X; */
+ ctx->seen |= SEEN_XREG;
+ PPC_CMPWI(r_X, 0);
+ if (ctx->pc_ret0 != -1) {
+ PPC_BCC(COND_EQ, addrs[ctx->pc_ret0]);
+ } else {
+ PPC_BCC_SHORT(COND_NE, (ctx->idx*4)+12);
+ PPC_LI(r_ret, 0);
+ PPC_JMP(exit_addr);
+ }
+ PPC_DIVWU(r_scratch1, r_A, r_X);
+ PPC_MUL(r_scratch1, r_X, r_scratch1);
+ PPC_SUB(r_A, r_A, r_scratch1);
+ break;
+ case BPF_S_ALU_MOD_K: /* A %= K; */
+ PPC_LI32(r_scratch2, K);
+ PPC_DIVWU(r_scratch1, r_A, r_scratch2);
+ PPC_MUL(r_scratch1, r_scratch2, r_scratch1);
+ PPC_SUB(r_A, r_A, r_scratch1);
+ break;
case BPF_S_ALU_DIV_X: /* A /= X; */
ctx->seen |= SEEN_XREG;
PPC_CMPWI(r_X, 0);
--
1.8.1.5
^ permalink raw reply related
* [PATCH v2 1/2] powerpc: net: filter: fix DIVWU instruction opcode
From: Vladimir Murzin @ 2013-09-28 8:22 UTC (permalink / raw)
To: linuxppc-dev
Cc: matt, netdev, Vladimir Murzin, dborkman, edumazet, paulus, davem
Currently DIVWU stands for *signed* divw opcode:
7d 2a 4b 96 divwu r9,r10,r9
7d 2a 4b d6 divw r9,r10,r9
Use the *unsigned* divw opcode for DIVWU.
Suggested-by: Vassili Karpov <av1474@comtv.ru>
Reviewed-by: Vassili Karpov <av1474@comtv.ru>
Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
Acked-by: Matt Evans <matt@ozlabs.org>
---
Changelog
v1->v2
Added credit to Vassili Karpov (malc) who kindly reviewed generated assembly
[1] and highlighted usage of signed division.
Note: temporary, for technical reason, he's not able to receive email.
[1]http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg71635.html
arch/powerpc/include/asm/ppc-opcode.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index eccfc16..0a4a683 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -171,7 +171,7 @@
#define PPC_INST_MULLW 0x7c0001d6
#define PPC_INST_MULHWU 0x7c000016
#define PPC_INST_MULLI 0x1c000000
-#define PPC_INST_DIVWU 0x7c0003d6
+#define PPC_INST_DIVWU 0x7c000396
#define PPC_INST_RLWINM 0x54000000
#define PPC_INST_RLDICR 0x78000004
#define PPC_INST_SLW 0x7c000030
--
1.8.1.5
^ permalink raw reply related
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-28 3:05 UTC (permalink / raw)
To: Yinghai Lu
Cc: linux-pci@vger.kernel.org, linuxppc-dev, Linux Kernel list,
Rafael J. Wysocki, Bjorn Helgaas, Linus Torvalds
In-Reply-To: <CAE9FiQV8ipnQ_vV68S-QHMrUrQTWQT_XObxyBeR=dOuw6fG1eg@mail.gmail.com>
On Fri, 2013-09-27 at 16:44 -0700, Yinghai Lu wrote:
> > Thus the port driver bails out before calling pci_set_master(). The fix
> > is to call pci_set_master() unconditionally. However that lead me to
> > find to a few interesting oddities in that port driver code:
>
> can we revert that partially change ? aka we should check get_port....
> at first...
>
> like attached.
In the meantime, can you properly submit the other one with the warning
to Linus ? It will make things more robust overall...
Also, please read my other comments. I think we are treading on very
fragile ground with that whole business of potentially disabling bridges
in the pcieport driver ...
Cheers,
Ben.
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Yinghai Lu @ 2013-09-27 23:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-pci@vger.kernel.org, linuxppc-dev, Linux Kernel list,
Rafael J. Wysocki, Bjorn Helgaas, Linus Torvalds
In-Reply-To: <1380323960.27811.67.camel@pasglop>
[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]
[+ Rafael]
On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:
>
>> ok, please if you are ok attached one instead. It will print some warning about
>> driver skipping pci_set_master, so we can catch more problem with drivers.
>
> Except that the message is pretty cryptic :-) Especially since the
> driver causing the message to be printed is not the one that did
> the mistake in the first place, it's the next one coming up that
> trips the warning.
>
> In any case, the root cause is indeed the PCIe port driver:
>
> We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
> and pcie_ports_auto is true, so we end up with capabilities set to 0.
in
| commit fe31e69740eddc7316071ed5165fed6703c8cd12
| Author: Rafael J. Wysocki <rjw@sisk.pl>
| Date: Sun Dec 19 15:57:16 2010 +0100
|
| PCI/PCIe: Clear Root PME Status bits early during system resume
|
| I noticed that PCI Express PMEs don't work on my Toshiba Portege R500
| after the system has been woken up from a sleep state by a PME
| (through Wake-on-LAN). After some investigation it turned out that
| the BIOS didn't clear the Root PME Status bit in the root port that
| received the wakeup PME and since the Requester ID was also set in
| the port's Root Status register, any subsequent PMEs didn't trigger
| interrupts.
|
| This problem can be avoided by clearing the Root PME Status bits in
| all PCI Express root ports during early resume. For this purpose,
| add an early resume routine to the PCIe port driver and make this
| driver be always registered, even if pci_ports_disable is set (in
| which case the driver's only function is to provide the early
| resume callback).
|
|
|@@ -349,15 +349,18 @@ int pcie_port_device_register(struct pci_dev *dev)
| int status, capabilities, i, nr_service;
| int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
|- /* Get and check PCI Express port services */
|- capabilities = get_port_device_capability(dev);
|- if (!capabilities)
|- return -ENODEV;
|-
| /* Enable PCI Express port device */
| status = pci_enable_device(dev);
| if (status)
| return status;
|+
|+ /* Get and check PCI Express port services */
|+ capabilities = get_port_device_capability(dev);
|+ if (!capabilities) {
|+ pcie_no_aspm();
|+ return 0;
|+ }
|+
| pci_set_master(dev);
| /*
| * Initialize service irqs. Don't use service devices that
>
> Thus the port driver bails out before calling pci_set_master(). The fix
> is to call pci_set_master() unconditionally. However that lead me to
> find to a few interesting oddities in that port driver code:
can we revert that partially change ? aka we should check get_port....
at first...
like attached.
Thanks
Yinghai
[-- Attachment #2: fix_pci_set_master_port_pcie.patch --]
[-- Type: application/octet-stream, Size: 784 bytes --]
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 31063ac..1ee6f16 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -362,16 +362,16 @@ int pcie_port_device_register(struct pci_dev *dev)
int status, capabilities, i, nr_service;
int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
- /* Enable PCI Express port device */
- status = pci_enable_device(dev);
- if (status)
- return status;
-
/* Get and check PCI Express port services */
capabilities = get_port_device_capability(dev);
if (!capabilities)
return 0;
+ /* Enable PCI Express port device */
+ status = pci_enable_device(dev);
+ if (status)
+ return status;
+
pci_set_master(dev);
/*
* Initialize service irqs. Don't use service devices that
^ permalink raw reply related
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-27 23:19 UTC (permalink / raw)
To: Yinghai Lu
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CAE9FiQWm7ALPO5k_2nV+fuZw4oNNiw+_PEx8EWkb52ymN+EYUg@mail.gmail.com>
On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:
> ok, please if you are ok attached one instead. It will print some warning about
> driver skipping pci_set_master, so we can catch more problem with drivers.
Except that the message is pretty cryptic :-) Especially since the
driver causing the message to be printed is not the one that did
the mistake in the first place, it's the next one coming up that
trips the warning.
In any case, the root cause is indeed the PCIe port driver:
We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
and pcie_ports_auto is true, so we end up with capabilities set to 0.
Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:
- If capabilities is 0, it returns after enabling the device and
doesn't disable it. But if it fails for any other reason later on (such
as failing to enable interrupts), it *will* disable the device. This is
inconsistent. In fact, if it had disabled the device as a result of the
0 capabilities, the problem would also not have happened (the subsequent
enable_bridge done by the e1000e driver would have done the right
thing). I've tested "fixing" that instead of moving the set_master call
and it fixes my problem as well.
- In get_port_device_capability(), all capabilities are gated by a
combination of the bit in cap_mask and a corresponding HW check of
the presence of the relevant PCIe capability or similar... except
for the VC one, which is solely read from the HW capability. That means
that the platform pcie_port_platform_notify() has no ability to prevent
the VC capability (so if I have a broken bridge that advertises it but
my platform wants to block it, it can't).
- I am quite nervous with the PCIe port driver disabling bridges. I
understand the intent but what if that bridge has some system device
behind it ? Something you don't even know about (used by ACPI, behind an
ISA bridge for example ?).
I think disabling bridges is a VERY risky proposition at all times
(including during PM) and I don't think the port driver should do it.
Maybe a more robust approach would be to detect one way or another that
the bridge was already enabled and only disable it if it wasn't or
something along those lines (ie ,restore it in the state it was)...
This is not my problem right now of course (in my case the bridge was
disabled to begin with) but I have a long experience with system stuff
hiding behind bridges and the code as it is written makes me a bit
nervous.
Cheers,
Ben.
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Yinghai Lu @ 2013-09-27 22:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <1380321480.27811.53.camel@pasglop>
[-- Attachment #1: Type: text/plain, Size: 942 bytes --]
On Fri, Sep 27, 2013 at 3:38 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
>> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>>
>> > Wouldn't it be better to simply have pci_enable_device() always set bus
>> > master on a bridge? I don't see any case where it makes sense to have
>> > an enabled bridge without the master bit set on it...
>>
>> Do you mean attached?
>
> So this patch works and fixes the problem. I think it makes the whole
> thing more robust and should be applied.
good.
>
> I still don't know why the bridge doesn't get enabled properly without
> it yes, tracking it down (the machine in question takes a LONG time to
> reboot :-)
ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.
Thanks
Yinghai
[-- Attachment #2: pci_set_master_again_v2.patch --]
[-- Type: application/octet-stream, Size: 1420 bytes --]
Subject: [PATCH] PCI: Workaround missing pci_set_master in pci drivers
BenH found:
| 928bea964827d7824b548c1f8e06eccbbc4d0d7d
| PCI: Delay enabling bridges until they're needed
break PCI on powerpc. The reason is that the PCIe port driver will
call pci_enable_device() on the bridge, so device enabled (but skip
pci_set_master somehow).
Because of that, pci_enable_bridge() later on (called as a result of the
child device driver doing pci_enable_device) will see the bridge as
already enabled and will not call pci_set_master() on it.
Fixed by add checking in pci_enable_bridge, and call pci_set_master
if driver skip that.
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
drivers/pci/pci.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
pci_enable_bridge(dev->bus->self);
- if (pci_is_enabled(dev))
+ if (pci_is_enabled(dev)) {
+ if (!dev->is_busmaster) {
+ dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
+ pci_set_master(dev);
+ }
return;
+ }
+
retval = pci_enable_device(dev);
if (retval)
dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-27 22:38 UTC (permalink / raw)
To: Yinghai Lu
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CAE9FiQXXvkfTak6oEpUog_RYs_Gwpdji7N5UJ=R11Q61==N97A@mail.gmail.com>
On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>
> > Wouldn't it be better to simply have pci_enable_device() always set bus
> > master on a bridge? I don't see any case where it makes sense to have
> > an enabled bridge without the master bit set on it...
>
> Do you mean attached?
So this patch works and fixes the problem. I think it makes the whole
thing more robust and should be applied.
I still don't know why the bridge doesn't get enabled properly without
it yes, tracking it down (the machine in question takes a LONG time to
reboot :-)
Cheers,
Ben.
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-27 22:15 UTC (permalink / raw)
To: Yinghai Lu
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CAE9FiQW+b7OtN4hJc6uygzbqzaCOkn3WcLDgSg5isHAJfdgmwg@mail.gmail.com>
On Fri, 2013-09-27 at 10:44 -0700, Yinghai Lu wrote:
> | /* Get and check PCI Express port services */
> | capabilities = get_port_device_capability(dev);
> | if (!capabilities)
> | return 0;
> |
> | pci_set_master(dev);
>
> so how come that pci_set_master is not called for powerpc?
>
> Can you send out lspci -vvxxx with current linus-tree and v3.11?
Ah good point. It should have ... except that there are a number of ways
for get_port_device_capability() to fail and that should *not* leave the
bridge without the bus master capability set.
However I don't think that's what's happening here. I'll have to dig
more, will get back to you.
Cheers,
Ben.
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-27 22:00 UTC (permalink / raw)
To: Yinghai Lu
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CAE9FiQXXvkfTak6oEpUog_RYs_Gwpdji7N5UJ=R11Q61==N97A@mail.gmail.com>
On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>
> > Wouldn't it be better to simply have pci_enable_device() always set bus
> > master on a bridge? I don't see any case where it makes sense to have
> > an enabled bridge without the master bit set on it...
>
> Do you mean attached?
That's an option. I was thinking making pci_enable_device() itself
enable bus master on a bridge but yes, you approach should work.
I'm digging a bit more to figure out what went wrong in the
pcie port driver since that's interesting in its own right and I'll then
test your patch which I think is a more robust approach.
Cheers,
Ben.
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Yinghai Lu @ 2013-09-27 21:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <1380318405.27811.37.camel@pasglop>
[-- Attachment #1: Type: text/plain, Size: 316 bytes --]
On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Wouldn't it be better to simply have pci_enable_device() always set bus
> master on a bridge? I don't see any case where it makes sense to have
> an enabled bridge without the master bit set on it...
Do you mean attached?
[-- Attachment #2: pci_set_master_again.patch --]
[-- Type: application/octet-stream, Size: 510 bytes --]
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e8ccf6c..4eff99b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1156,11 +1156,13 @@ static void pci_enable_bridge(struct pci_dev *dev)
pci_enable_bridge(dev->bus->self);
if (pci_is_enabled(dev))
- return;
+ goto out; /* some other driver could skip pci_set_master ! */
retval = pci_enable_device(dev);
if (retval)
dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
retval);
+
+out:
pci_set_master(dev);
}
^ permalink raw reply related
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Benjamin Herrenschmidt @ 2013-09-27 21:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Yinghai Lu,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CA+55aFyDfMdAeaniQwkoeW6F-xd3JxrzGYwtFb6N9+NA6aRCMg@mail.gmail.com>
On Fri, 2013-09-27 at 10:10 -0700, Linus Torvalds wrote:
> > So i would like to use the first way that you suggest : call pci_set_master
> > PCIe port driver.
>
> So I have to say, that if we can fix this with just adding a single
> new pci_set_master() call, we should do that before we decide to
> revert.
>
> If other, bigger issues then come up, we can decide to revert. But if
> there's a one-liner fix, let's just do that first, ok?
>
> Mind sending a patch?
Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v4] powerpc 8xx: Fixing issue with CONFIG_PIN_TLB
From: Scott Wood @ 2013-09-27 21:43 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <201309240818.r8O8IdF9016374@localhost.localdomain>
On Tue, 2013-09-24 at 10:18 +0200, Christophe Leroy wrote:
> Activating CONFIG_PIN_TLB is supposed to pin the IMMR and the first three
> 8Mbytes pages. But the setting of MD_CTR to a pinnable entry was missing before
> the pinning of the third 8Mb page. As the index is decremented module 28
> (MD_RSV4D is set) after every DTLB update, the third 8Mbytes page was
> not pinned.
The examples you showed weren't quite modulo 28, more like "if (x >= 28)
x -= 4". I'll fix up the changelog on applying, to read something like
"As the index is decremented to a value within the first 28 entries
(MD_RSV4D is set)...".
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc/p1010rdb:update dts to adapt to both old and new p1010rdb
From: Scott Wood @ 2013-09-27 21:35 UTC (permalink / raw)
To: Zhao Qiang; +Cc: linuxppc-dev, Shengzhou Liu
In-Reply-To: <1379816919-13851-1-git-send-email-B45475@freescale.com>
On Sun, 2013-09-22 at 10:28 +0800, Zhao Qiang wrote:
> P1010rdb-pa and p1010rdb-pb have different phy interrupts.
> So update dts to adapt to both p1010rdb-pa and p1010rdb-pb.
>
> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
> Signed-off-by: Zhao Qiang <B45475@freescale.com>
> ---
> arch/powerpc/boot/dts/p1010rdb-pa.dts | 33 ++++++++++++++++++
> arch/powerpc/boot/dts/p1010rdb-pb.dts | 33 ++++++++++++++++++
> arch/powerpc/boot/dts/p1010rdb.dts | 66 -----------------------------------
> arch/powerpc/boot/dts/p1010rdb.dtsi | 51 ++++++++++++++++++++++++---
> 4 files changed, 112 insertions(+), 71 deletions(-)
> create mode 100644 arch/powerpc/boot/dts/p1010rdb-pa.dts
> create mode 100644 arch/powerpc/boot/dts/p1010rdb-pb.dts
> delete mode 100644 arch/powerpc/boot/dts/p1010rdb.dts
What about p1010rdb_36b.dts?
-Scott
^ permalink raw reply
* Re: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and altivec idle
From: Scott Wood @ 2013-09-27 21:33 UTC (permalink / raw)
To: Wang Dongsheng-B40534
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org,
Bhushan Bharat-R65777
In-Reply-To: <ABB05CD9C9F68C46A5CEDC7F15439259010911C2@039-SN2MPN1-021.039d.mgd.msft.net>
On Thu, 2013-09-26 at 22:34 -0500, Wang Dongsheng-B40534 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, September 27, 2013 5:37 AM
> > To: Wang Dongsheng-B40534
> > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and
> > altivec idle
> >
> > On Thu, 2013-09-26 at 01:18 -0500, Wang Dongsheng-B40534 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Bhushan Bharat-R65777
> > > > Sent: Thursday, September 26, 2013 12:23 PM
> > > > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > Subject: RE: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state
> > > > and altivec idle
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang Dongsheng-B40534
> > > > > Sent: Thursday, September 26, 2013 8:02 AM
> > > > > To: Wood Scott-B07421
> > > > > Cc: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org
> > > > > Subject: RE: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state
> > > > > and altivec idle
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Thursday, September 26, 2013 1:57 AM
> > > > > > To: Wang Dongsheng-B40534
> > > > > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; linuxppc-
> > > > > > dev@lists.ozlabs.org
> > > > > > Subject: Re: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20
> > > > > > state and altivec idle
> > > > > >
> > > > > > On Wed, 2013-09-25 at 03:10 -0500, Wang Dongsheng-B40534 wrote:
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Bhushan Bharat-R65777
> > > > > > > > Sent: Wednesday, September 25, 2013 2:23 PM
> > > > > > > > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > > > > > > > Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> > > > > > > > Subject: RE: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20
> > > > > > > > state and altivec idle
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > > > > > > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On
> > > > > > > > > bounces+Behalf Of Dongsheng
> > > > > > > > > Wang
> > > > > > > > > Sent: Tuesday, September 24, 2013 2:59 PM
> > > > > > > > > To: Wood Scott-B07421
> > > > > > > > > Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> > > > > > > > > Subject: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20
> > > > > > > > > state and altivec idle
> > > > > > > > >
> > > > > > > > > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > > > > > > >
> > > > > > > > > Add a sys interface to enable/diable pw20 state or altivec
> > > > > > > > > idle, and control the wait entry time.
> > > > > > > > >
> > > > > > > > > Enable/Disable interface:
> > > > > > > > > 0, disable. 1, enable.
> > > > > > > > > /sys/devices/system/cpu/cpuX/pw20_state
> > > > > > > > > /sys/devices/system/cpu/cpuX/altivec_idle
> > > > > > > > >
> > > > > > > > > Set wait time interface:(Nanosecond)
> > > > > > > > > /sys/devices/system/cpu/cpuX/pw20_wait_time
> > > > > > > > > /sys/devices/system/cpu/cpuX/altivec_idle_wait_time
> > > > > > > > > Example: Base on TBfreq is 41MHZ.
> > > > > > > > > 1~47(ns): TB[63]
> > > > > > > > > 48~95(ns): TB[62]
> > > > > > > > > 96~191(ns): TB[61]
> > > > > > > > > 192~383(ns): TB[62]
> > > > > > > > > 384~767(ns): TB[60]
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > Signed-off-by: Wang Dongsheng
> > > > > > > > > <dongsheng.wang@freescale.com>
> > > > > > > > > ---
> > > > > > > > > *v4:
> > > > > > > > > Move code from 85xx/common.c to kernel/sysfs.c.
> > > > > > > > >
> > > > > > > > > Remove has_pw20_altivec_idle function.
> > > > > > > > >
> > > > > > > > > Change wait "entry_bit" to wait time.
> > > > > > > > >
> > > > > > > > > arch/powerpc/kernel/sysfs.c | 291
> > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > 1 file changed, 291 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/powerpc/kernel/sysfs.c
> > > > > > > > > b/arch/powerpc/kernel/sysfs.c index 27a90b9..23fece6
> > > > > > > > > 100644
> > > > > > > > > --- a/arch/powerpc/kernel/sysfs.c
> > > > > > > > > +++ b/arch/powerpc/kernel/sysfs.c
> > > > > > > > > @@ -85,6 +85,279 @@ __setup("smt-snooze-delay=",
> > > > > > > > > setup_smt_snooze_delay);
> > > > > > > > >
> > > > > > > > > #endif /* CONFIG_PPC64 */
> > > > > > > > >
> > > > > > > > > +#ifdef CONFIG_FSL_SOC
> > > > > > > > > +#define MAX_BIT 63
> > > > > > > > > +
> > > > > > > > > +static u64 pw20_wt;
> > > > > > > > > +static u64 altivec_idle_wt;
> > > > > > > > > +
> > > > > > > > > +static unsigned int get_idle_ticks_bit(u64 ns) {
> > > > > > > > > + u64 cycle;
> > > > > > > > > +
> > > > > > > > > + cycle = div_u64(ns, 1000 / tb_ticks_per_usec);
> > > > > > > >
> > > > > > > > When tb_ticks_per_usec > 1000 (timebase frequency > 1GHz)
> > > > > > > > then this will always be ns, which is not correct, no?
> > > > > >
> > > > > > Actually it'll be a divide by zero in that case.
> > > > > >
> > > > > tb_ticks_per_usec = ppc_tb_freq / 1000000; Means TB freq should be
> > > > > more than 1MHZ.
> > > > >
> > > > > if ppc_tb_freq less than 1000000, the tb_ticks_per_usec will be a
> > > > > divide by zero.
> > > > > If this condition is established, I think kernel cannot work as a
> > > > normal.
> > > > >
> > > > > So I think we need to believe that the variable is not zero.
> > > >
> > > > We do believe it is non-zero but greater than 1000 :)
> > > >
> > > > > And I think TB freq
> > > > > should not less than 1MHZ on PPC platform, because if TB freq less
> > > > > than 1MHZ, the precision time will become very poor and system
> > > > > response time will be slower.
> > > >
> > > > Not sure what you are describing here related to divide by zero we
> > > > are mentioning.
> > > > You are talking about if tb_ticks_per_usec is ZERO and we are
> > > > talking about if (1000/tb_ticks_per_usec) will be zero.
> > > >
> > > > BTW, div_u64() handle the case where divider is zero.
> >
> > How? When I checked yesterday it looked like div_u64() mapped to a
> > hardware division on 64-bit targets. In any case it's not good to rely
> > on such behavior.
> >
> > > cycle = div_u64(ns, 1000 / tb_ticks_per_usec); For this, I think we
> > > were discussing the two issues:
> > >
> > > 1. Scott talking about, when the tb_ticks_per_usec is a zero.
> >
> > No I wasn't. I was talking about when tb_ticks_per_usec > 1000, and thus
> > "1000 / tb_ticks_per_usec" is zero. You said that the result would be ns
> > in that case.
> >
> > > 2. You are talking about 1000/tb_ticks_per_usec is a zero.
> > > This situation is about TB freq > 1GHZ.
> > >
> > > I will fix this issue. Like I said before, "If timebase frequency >
> > > 1GHz, this should be "tb_ticks_per_usec / 1000" and to get
> > tb_ticks_per_nsec.
> > > This should be changed to "cycle = ns * tb_ticks_per_nsec;""
> > >
> > > #define TB_FREQ_1GHZ 1000
> > >
> > > If (tb_ticks_per_usec > TB_FREQ_1GHZ)
> > > cycle = ns * (tb_ticks_per_usec / 1000); else
> > > cycle = div_u64(ns, 1000 / tb_ticks_per_usec);
> > >
> > > how about this? :)
> >
> > I suggested an alternative to satisfy your complaint that it's hard to
> > test one of those if/else branches.
> >
> > Plus, your version will be quite inaccurate if (e.g.) tb_ticks_per_usec
> > is 501, or 1999.
> >
> cycle = div_u64(ns * tb_ticks_per_usec, 1000); It's look good.
> But why we need:
> if (ns >= 10000)
> cycle = ((ns + 500) / 1000) * tb_ticks_per_usec;
> ?
>
> I think "cycle = div_u64(ns * tb_ticks_per_usec, 1000)" is good enough. :)
It's to deal with overflow if a very large value of ns is provided
(and/or tb_ticks_per_usec is larger than we expect).
-Scott
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Yinghai Lu @ 2013-09-27 17:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <CAE9FiQVkW7d7vFx_kPJG-FEvO71SZjZmpR+dnwVm61wkYK=0YA@mail.gmail.com>
On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> Hi Linus, Yinghai !
>>
>> Please consider reverting:
>>
>> 928bea964827d7824b548c1f8e06eccbbc4d0d7d
>> PCI: Delay enabling bridges until they're needed
>>
>> (I'd suggest to revert now and maybe merge a better patch later)
>>
>> This breaks PCI on the PowerPC "powernv" platform (which is booted via
>> kexec) and probably x86 as well under similar circumstances. It will
>> basically break PCIe if the bus master bit of the bridge isn't set at
>> boot (by the firmware for example, or because kexec'ing cleared it).
>>
>> The reason is that the PCIe port driver will call pci_enable_device() on
>> the bridge (on everything under the sun actually), which will marked the
>> device enabled (but will not do a set_master).
>>
>> Because of that, pci_enable_bridge() later on (called as a result of the
>> child device driver doing pci_enable_device) will see the bridge as
>> already enabled and will not call pci_set_master() on it.
Ben,
looks like PCIe port driver does call pci_enable_device AND pci_set_master()
| int pcie_port_device_register(struct pci_dev *dev)
| {
| int status, capabilities, i, nr_service;
| int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
| /* Enable PCI Express port device */
| status = pci_enable_device(dev);
| if (status)
| return status;
|
| /* Get and check PCI Express port services */
| capabilities = get_port_device_capability(dev);
| if (!capabilities)
| return 0;
|
| pci_set_master(dev);
so how come that pci_set_master is not called for powerpc?
Can you send out lspci -vvxxx with current linus-tree and v3.11?
Yinghai
^ permalink raw reply
* Re: [PATCH] powerpc/mpic: Disable preemption when calling mpic_processor_id()
From: Scott Wood @ 2013-09-27 17:26 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <D2DE40C4-94A5-425C-9A67-75BA6F4CD145@kernel.crashing.org>
On Fri, 2013-09-27 at 12:09 -0500, Kumar Gala wrote:
> On Sep 27, 2013, at 11:15 AM, Scott Wood wrote:
>
> > On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
> >> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
> >>
> >>> Otherwise, we get a debug traceback due to the use of
> >>> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> >>> mpic_host_map() is just looking for a default CPU, so it doesn't matter
> >>> if we migrate after getting the CPU ID.
> >>>
> >>> Signed-off-by: Scott Wood <scottwood@freescale.com>
> >>> ---
> >>> arch/powerpc/sysdev/mpic.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> >>> index 1be54fa..bdcb858 100644
> >>> --- a/arch/powerpc/sysdev/mpic.c
> >>> +++ b/arch/powerpc/sysdev/mpic.c
> >>> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> >>> * is done here.
> >>> */
> >>> if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> >>> + int cpu;
> >>> +
> >>> + preempt_disable();
> >>> + cpu = mpic_processor_id(mpic);
> >>> + preempt_enable();
> >>> +
> >>
> >> Any reason you didn't stick this inside of mpic_processor_id() ?
> >
> > Because the debug check might be valid for other callers and we don't
> > want to defeat it. In this caller it's used only as a heuristic and
> > thus it doesn't matter if we re-enable preemption before using the
> > result.
>
> Gotcha, I think you should reword the commit message. Reading it makes
> me think that preemption should be disabled for all mpic_processor_id()
> calls.
It should be disabled for all mpic_processor_id() calls -- but some
calls will need preemption to be disabled for longer than just the call
to mpic_processor_id().
I did mention in the commit message why mpic_host_map() is unusual.
BTW, I wonder why we don't use mpic_set_affinity (and thus
irq_choose_cpu), both here and in the non-MPIC_NO_RESET case. Though I
usually notice IRQs being round robinned -- I guess userspace is writing
to the affinity in proc? Or is something elsewhere in the kernel
calling set_affinity at some point?
Another thing I just noticed while looking at IRQ_DESTINATION-related
code, in mpic_teardown_this_cpu(), what stops it from removing the only
CPU that an interrupt is targeted at?
-Scott
^ permalink raw reply
* Re: [PATCH][v5] powerpc/mpc85xx:Add initial device tree support of T104x
From: Scott Wood @ 2013-09-27 17:14 UTC (permalink / raw)
To: Prabhakar Kushwaha
Cc: Varun Sethi, linuxppc-dev, Poonam Aggrwal, Priyanka Jain
In-Reply-To: <1380103852-15937-1-git-send-email-prabhakar@freescale.com>
On Wed, 2013-09-25 at 15:40 +0530, Prabhakar Kushwaha wrote:
> The QorIQ T1040/T1042 processor support four integrated 64-bit e5500 PA
> processor cores with high-performance data path acceleration architecture
> and network peripheral interfaces required for networking & telecommunications.
>
> T1042 personality is a reduced personality of T1040 without Integrated 8-port
> Gigabit Ethernet switch.
>
> The T1040/T1042 SoC includes the following function and features:
>
> - Four e5500 cores, each with a private 256 KB L2 cache
> - 256 KB shared L3 CoreNet platform cache (CPC)
> - Interconnect CoreNet platform
> - 32-/64-bit DDR3L/DDR4 SDRAM memory controller with ECC and interleaving
> support
> - Data Path Acceleration Architecture (DPAA) incorporating acceleration
> for the following functions:
> - Packet parsing, classification, and distribution
> - Queue management for scheduling, packet sequencing, and congestion
> management
> - Cryptography Acceleration (SEC 5.0)
> - RegEx Pattern Matching Acceleration (PME 2.2)
> - IEEE Std 1588 support
> - Hardware buffer management for buffer allocation and deallocation
> - Ethernet interfaces
> - Integrated 8-port Gigabit Ethernet switch (T1040 only)
> - Four 1 Gbps Ethernet controllers
> - Two RGMII interfaces or one RGMII and one MII interfaces
> - High speed peripheral interfaces
> - Four PCI Express 2.0 controllers running at up to 5 GHz
> - Two SATA controllers supporting 1.5 and 3.0 Gb/s operation
> - Upto two QSGMII interface
> - Upto six SGMII interface supporting 1000 Mbps
> - One SGMII interface supporting upto 2500 Mbps
> - Additional peripheral interfaces
> - Two USB 2.0 controllers with integrated PHY
> - SD/eSDHC/eMMC
> - eSPI controller
> - Four I2C controllers
> - Four UARTs
> - Four GPIO controllers
> - Integrated flash controller (IFC)
> - Change this to LCD/ HDMI interface (DIU) with 12 bit dual data rate
> - TDM interface
> - Multicore programmable interrupt controller (PIC)
> - Two 8-channel DMA engines
> - Single source clocking implementation
> - Deep Sleep power implementaion (wakeup from GPIO/Timer/Ethernet/USB)
>
> Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com>
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
> ---
> Based upon git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git
> Branch next
>
> Changes for v2: Incorporated Scott's comments
> - Update t1040si-post.dtsi
> - update clock device tree node as per
> http://patchwork.ozlabs.org/patch/274134/
> - removed DMA node, It will be added later as per
> http://patchwork.ozlabs.org/patch/271238/
> - Updated display compatible field
>
> Changes for v3: Incorporated Scott's comments
> - Updated soc compatible field
> - updated clock compatible field
>
> Changes for v4: Sending as it is
> Changes for v5: Sending as it is
This should have been marked as a numbered patch set along with
"powerpc/fsl-booke: Add initial T104x_QDS board support".
> + display@180000 {
> + compatible = "fsl,t1040-diu", "fsl,diu";
> + reg = <0x180000 1000>;
> + interrupts = <74 2 0 0>;
> + };
Whitespace
> +l2switch@800000 {
> + compatible = "fsl,t1040-l2s";
> + reg = <0x800000 0x400000>;
> +};
s/l2s/l2-switch/
Is there a better name for this? Is there a version number or register?
There should be a binding, even if it's just a trivial one establishing
the compatible name, similar to
Documentation/devicetree/bindings/i2c/trivial-devices.txt
-Scott
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Linus Torvalds @ 2013-09-27 17:10 UTC (permalink / raw)
To: Yinghai Lu
Cc: Bjorn Helgaas, linuxppc-dev, Linux Kernel list,
linux-pci@vger.kernel.org
In-Reply-To: <CAE9FiQVkW7d7vFx_kPJG-FEvO71SZjZmpR+dnwVm61wkYK=0YA@mail.gmail.com>
On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> So i would like to use the first way that you suggest : call pci_set_master
> PCIe port driver.
So I have to say, that if we can fix this with just adding a single
new pci_set_master() call, we should do that before we decide to
revert.
If other, bigger issues then come up, we can decide to revert. But if
there's a one-liner fix, let's just do that first, ok?
Mind sending a patch?
Linus
^ permalink raw reply
* Re: [PATCH] powerpc/mpic: Disable preemption when calling mpic_processor_id()
From: Kumar Gala @ 2013-09-27 17:09 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <1380298512.24959.384.camel@snotra.buserror.net>
On Sep 27, 2013, at 11:15 AM, Scott Wood wrote:
> On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
>> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
>>=20
>>> Otherwise, we get a debug traceback due to the use of
>>> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
>>> mpic_host_map() is just looking for a default CPU, so it doesn't =
matter
>>> if we migrate after getting the CPU ID.
>>>=20
>>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>>> ---
>>> arch/powerpc/sysdev/mpic.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>=20
>>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
>>> index 1be54fa..bdcb858 100644
>>> --- a/arch/powerpc/sysdev/mpic.c
>>> +++ b/arch/powerpc/sysdev/mpic.c
>>> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain =
*h, unsigned int virq,
>>> * is done here.
>>> */
>>> if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
>>> + int cpu;
>>> +
>>> + preempt_disable();
>>> + cpu =3D mpic_processor_id(mpic);
>>> + preempt_enable();
>>> +
>>=20
>> Any reason you didn't stick this inside of mpic_processor_id() ?
>=20
> Because the debug check might be valid for other callers and we don't
> want to defeat it. In this caller it's used only as a heuristic and
> thus it doesn't matter if we re-enable preemption before using the
> result.
Gotcha, I think you should reword the commit message. Reading it makes =
me think that preemption should be disabled for all mpic_processor_id() =
calls.
- k=
^ permalink raw reply
* Re: [PATCH V4 3/3] powerpc/85xx: Add TWR-P1025 board support
From: Scott Wood @ 2013-09-27 17:03 UTC (permalink / raw)
To: Xie Xiaobo-R63061
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org,
Johnston Michael-R49610
In-Reply-To: <69EC9ED88E3CC04094A78F8074A7986D5FF91C@039-SN1MPN1-003.039d.mgd.msft.net>
On Wed, 2013-09-25 at 04:50 -0500, Xie Xiaobo-R63061 wrote:
> > > +#if defined(CONFIG_SERIAL_QE)
> > > + /* On P1025TWR board, the UCC7 acted as UART port.
> > > + * However, The UCC7's CTS pin is low level in default,
> > > + * it will impact the transmission in full duplex
> > > + * communication. So disable the Flow control pin PA18.
> > > + * The UCC7 UART just can use RXD and TXD pins.
> > > + */
> > > + par_io_config_pin(0, 18, 0, 0, 0, 0); #endif
> >
> > Any reason not to do this unconditionally?
>
> This is a board issue, the code already have a condition - defined
> SERIAL_QE, and I will add a condition "if (machine_is(twr_p1025))".
My point was, is there any harm in doing this without regard to
CONFIG_SERIAL_QE?
-Scott
^ permalink raw reply
* Re: [PATCH V5 1/2] powerpc/85xx: Add QE common init function
From: Scott Wood @ 2013-09-27 17:00 UTC (permalink / raw)
To: Xie Xiaobo; +Cc: linuxppc-dev
In-Reply-To: <1380188253-4710-1-git-send-email-X.Xie@freescale.com>
On Thu, 2013-09-26 at 17:37 +0800, Xie Xiaobo wrote:
> +#ifdef CONFIG_QUICC_ENGINE
> +void __init mpc85xx_qe_init(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "fsl,qe");
> + if (!np) {
> + np = of_find_node_by_name(NULL, "qe");
> + if (!np) {
> + pr_err("%s: Could not find Quicc Engine node\n",
> + __func__);
> + return;
> + }
> + }
> +
> + qe_reset();
> + of_node_put(np);
You're missing the of_device_is_available() check:
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe");
> - if (!np) {
> - np = of_find_node_by_name(NULL, "qe");
> - if (!np)
> - return;
> - }
> -
> - if (!of_device_is_available(np)) {
> - of_node_put(np);
> - return;
> - }
> -
> - qe_reset();
> - of_node_put(np);
-Scott
^ permalink raw reply
* Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape
From: Scott Wood @ 2013-09-27 16:54 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Bjorn Helgaas, linux-pci, linuxppc-dev, Zang Roy-R61911
In-Reply-To: <1379502122-20792-2-git-send-email-Minghuan.Lian@freescale.com>
On Wed, 2013-09-18 at 19:02 +0800, Minghuan Lian wrote:
> @@ -592,6 +719,7 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
> #if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
>
> struct device_node *fsl_pci_primary;
> +extern const struct of_device_id fsl_pci_ids[];
Externs go in headers.
> -static int fsl_pci_probe(struct platform_device *pdev)
> +static int __init fsl_pci_probe(struct platform_device *pdev)
> {
> int ret;
> - struct device_node *node;
> + struct fsl_pci *pci;
> +
> + if (!of_device_is_available(pdev->dev.of_node)) {
> + dev_warn(&pdev->dev, "disabled\n");
> + return -ENODEV;
> + }
This should be dev_dbg().
> -#ifdef CONFIG_PM
> -static int fsl_pci_resume(struct device *dev)
> +static int __exit fsl_pci_remove(struct platform_device *pdev)
Why __exit? What happens if someone unbinds the PCI controller via
sysfs?
> +/*
> + * Structure of a PCI controller (host bridge)
> + */
> +struct fsl_pci {
> + struct list_head node;
> + int is_pcie;
bool is_pcie;
> +/* Return link status 0-> link, 1-> no link */
> +int fsl_pci_check_link(struct fsl_pci *pci);
bool
> +
> +/*
> + * The fsl_arch_* functions are arch hooks. Those functions are
> + * implemented as weak symbols so that they can be overridden by
> + * architecture specific code if needed.
> + */
> +
> +/* Return PCI64 DMA offset */
> +u64 fsl_arch_pci64_dma_offset(void);
Is this always guaranteed to exist?
> +/* Register PCI/PCIe controller to architecture system */
> +int __weak fsl_arch_pci_sys_register(struct fsl_pci *pci);
> +
> +/* Remove PCI/PCIe controller from architecture system */
> +void __weak fsl_arch_pci_sys_remove(struct fsl_pci *pci);
Why do these need to be weak? Won't there be exactly one implementation
per supported arch?
-Scott
^ permalink raw reply
* Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape
From: Scott Wood @ 2013-09-27 16:47 UTC (permalink / raw)
To: Lian Minghuan-b31939
Cc: linux-pci, Zang Roy-R61911, Minghuan Lian, Bjorn Helgaas,
linuxppc-dev
In-Reply-To: <52456C0C.2020504@freescale.com>
On Fri, 2013-09-27 at 19:29 +0800, Lian Minghuan-b31939 wrote:
> Hi All,
>
> Can anyone comment on my code or help to pick up?
Please break it up into multiple patches, each with one logical change
that is individually explained. It will be much easier to review that
way.
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc/mpic: Disable preemption when calling mpic_processor_id()
From: Scott Wood @ 2013-09-27 16:15 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <37986E99-BC99-4B67-9327-36CABA8E1A04@kernel.crashing.org>
On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
>
> > Otherwise, we get a debug traceback due to the use of
> > smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> > mpic_host_map() is just looking for a default CPU, so it doesn't matter
> > if we migrate after getting the CPU ID.
> >
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > ---
> > arch/powerpc/sysdev/mpic.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 1be54fa..bdcb858 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> > * is done here.
> > */
> > if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> > + int cpu;
> > +
> > + preempt_disable();
> > + cpu = mpic_processor_id(mpic);
> > + preempt_enable();
> > +
>
> Any reason you didn't stick this inside of mpic_processor_id() ?
Because the debug check might be valid for other callers and we don't
want to defeat it. In this caller it's used only as a heuristic and
thus it doesn't matter if we re-enable preemption before using the
result.
-Scott
^ permalink raw reply
* Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d
From: Yinghai Lu @ 2013-09-27 16:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org, Linus Torvalds,
linuxppc-dev, Linux Kernel list
In-Reply-To: <1380270519.27811.10.camel@pasglop>
On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Hi Linus, Yinghai !
>
> Please consider reverting:
>
> 928bea964827d7824b548c1f8e06eccbbc4d0d7d
> PCI: Delay enabling bridges until they're needed
>
> (I'd suggest to revert now and maybe merge a better patch later)
>
> This breaks PCI on the PowerPC "powernv" platform (which is booted via
> kexec) and probably x86 as well under similar circumstances. It will
> basically break PCIe if the bus master bit of the bridge isn't set at
> boot (by the firmware for example, or because kexec'ing cleared it).
>
> The reason is that the PCIe port driver will call pci_enable_device() on
> the bridge (on everything under the sun actually), which will marked the
> device enabled (but will not do a set_master).
>
> Because of that, pci_enable_bridge() later on (called as a result of the
> child device driver doing pci_enable_device) will see the bridge as
> already enabled and will not call pci_set_master() on it.
>
> Now, this could probably be fixed by simply doing pci_set_master() in
> the PCIe port driver, but I find the whole logic very fragile (anything
> that "enables" the bridge without setting master or for some reason
> clears master will forever fail to re-enable it).
>
> Maybe a better option is to unconditionally do pci_set_mater() in
> pci_enable_bridge(), ie, move the call to before the enabled test.
>
> However I am not too happy with that either. My experience with bridges
> is that if bus master isn't set, they will also fail to report AER
> errors and other similar upstream transactions. We might want to get
> these reported properly even if no downstream device got successfully
> enabled.
>
> So I think the premises of the patches are flawed, at least on PCI
> express, and we should stick to always enabling bridges (at least the
> bus master bit on them).
there is pci_set_master everywhere in pci drivers.
So i would like to use the first way that you suggest : call pci_set_master
PCIe port driver.
Thanks
Yinghai
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox