Good evening,
I recently tested the latest 3.10-rc release (rc4) on my powerpc based
routerboard which is currently running very stable on a 3.9 release.
During boot I immediately got a kernel panic.
I was able to get a log of the trace with the serial console.
[ 0.039522] PCI: Probing PCI hardware
[ 0.043159] PCI: Memory resource 0 not set for host bridge
/pci@e0008500 (domain 0)
[ 0.050778] PCI: Memory resource 0 not set for host bridge
/pci@e0008500 (domain 0)
[ 0.058855] PCI host bridge to bus 0000:00
[ 0.062938] pci_bus 0000:00: root bus resource [io 0x0000-0xffffff]
[ 0.069255] pci_bus 0000:00: root bus resource [mem 0x80000000-0x9fffffff]
[ 0.076131] pci_bus 0000:00: root bus resource [bus 00-ff]
[ 0.081639] Unable to handle kernel paging request for data at
address 0x00000f14
[ 0.089068] Faulting instruction address: 0xc0014d40
[ 0.094031] Oops: Kernel access of bad area, sig: 11 [#1]
[ 0.099420] MikroTik RouterBOARD 600 series
[ 0.103597] Modules linked in:
[ 0.106651] CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0-rc1+ #15
[ 0.112910] task: c782e000 ti: c7834000 task.ti: c7834000
[ 0.118303] NIP: c0014d40 LR: c0014dd8 CTR: c0014e10
[ 0.123262] REGS: c7835b40 TRAP: 0300 Not tainted (3.10.0-rc1+)
[ 0.129435] MSR: 00001032 <ME,IR,DR,RI> CR: 22000084 XER: 20000000
[ 0.135788] DAR: 00000f14, DSISR: 20000000
[ 0.139875]
GPR00: c0014e40 c7835bf0 c782e000 c0417000 00000000 00000000 00000004 00000000
GPR08: 00000000 00000000 00000000 00000037 22000084 00000000 c0003e90 00000000
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000045
GPR24: c0400000 c03f0000 c036134c c782de00 00000000 0000ea60 c782de00 c0417000
[ 0.169567] NIP [c0014d40] fsl_pcie_check_link.part.4+0x8/0x2c
[ 0.175380] LR [c0014dd8] fsl_pcie_check_link+0x74/0xac
[ 0.180591] Call Trace:
[ 0.183033] [c7835bf0] [00000025] 0x25 (unreliable)
[ 0.187909] [c7835d80] [c0014e40] fsl_indirect_read_config+0x30/0x8c
[ 0.194270] [c7835da0] [c019c108] pci_bus_read_config_dword+0x60/0x80
[ 0.200703] [c7835dc0] [c019df3c] pci_bus_read_dev_vendor_id+0x34/0x108
[ 0.207321] [c7835df0] [c022d394] pci_scan_single_device+0x58/0xc8
[ 0.213488] [c7835e20] [c019f068] pci_scan_slot+0x54/0x110
[ 0.218969] [c7835e40] [c019fc40] pci_scan_child_bus+0x28/0xdc
[ 0.224811] [c7835e60] [c0010aa4] pcibios_scan_phb+0x19c/0x210
[ 0.230640] [c7835e90] [c03b4658] pcibios_init+0x7c/0x114
[ 0.236025] [c7835ec0] [c00039a4] do_one_initcall+0x150/0x1a4
[ 0.241763] [c7835ef0] [c03b1888] kernel_init_freeable+0x114/0x1bc
[ 0.247938] [c7835f30] [c0003ea8] kernel_init+0x18/0x110
[ 0.253247] [c7835f40] [c000e098] ret_from_kernel_thread+0x64/0x6c
[ 0.259417] --- Exception: 0 at (null)
[ 0.259417] LR = (null)
[ 0.266287] Instruction dump:
[ 0.269246] 3d400006 614a0400 915f0028 3d40c040 912afc88 80010024
83e1001c 38210020
[ 0.276988] 7c0803a6 4e800020 812300ec 7c0004ac <80690f14> 0c030000
4c00012c 5463f6be
[ 0.284945] ---[ end trace 074b946a523243d5 ]---
Compare this to booting with 3.9
[ 0.039088] PCI: Probing PCI hardware
[ 0.043151] PCI host bridge to bus 0000:00
[ 0.047232] pci_bus 0000:00: root bus resource [io 0x0000-0xffffff]
[ 0.053546] pci_bus 0000:00: root bus resource [mem 0x80000000-0x9fffffff]
[ 0.060419] pci_bus 0000:00: root bus resource [bus 00-ff]
[ 0.068080] pci 0000:00:0d.0: BAR 0: assigned [mem 0x80000000-0x8000ffff]
[ 0.074854] pci 0000:00:0b.0: BAR 0: assigned [io 0x1000-0x10ff]
[ 0.080908] pci 0000:00:0b.0: BAR 1: assigned [mem 0x80010000-0x800100ff]
[ 0.103986] bio: create slab <bio-0> at 0
[ 0.108589] Freescale Elo / Elo Plus DMA driver
[ 0.115166] SCSI subsystem initialized
Especially I do not understand why this message shows up now
PCI: Memory resource 0 not set for host bridge
Does someone have an idea what's happening here?
I tested this also on rc1 and got the same panic there.
Please CC me on any replies since I am not subscribed to the list.
Thank you very much in advance,
Michael Guntsche
After bisecting I found the responsible commit.
50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
PCIe controllers
Reverting this commit allowed my board to boot again.
@Rojhalat: Please have a look at
http://marc.info/?l=linux-kernel&m=137071294204858&w=2
for my initial bugreport.
What I do not understand at all is why this is affecting my platform.
AFAIK there is no PCIe hardware on it AND I completely disabled PCIe
support in config.
Kind regards,
Mike
Hi Mike,
could you please try this patch:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
http://patchwork.ozlabs.org/patch/244515/
Rojhalat
On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
> After bisecting I found the responsible commit.
>
> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
> PCIe controllers
>
> Reverting this commit allowed my board to boot again.
>
> @Rojhalat: Please have a look at
> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
> for my initial bugreport.
>
> What I do not understand at all is why this is affecting my platform.
> AFAIK there is no PCIe hardware on it AND I completely disabled PCIe
> support in config.
>
> Kind regards,
> Mike
Good evening,
On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <[email protected]> wrote:
> Hi Mike,
>
> could you please try this patch:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> http://patchwork.ozlabs.org/patch/244515/
>
> Rojhalat
>
>
> On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
>> After bisecting I found the responsible commit.
>>
>> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
>> PCIe controllers
>>
>> Reverting this commit allowed my board to boot again.
>>
>> @Rojhalat: Please have a look at
>> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
>> for my initial bugreport.
>>
>> What I do not understand at all is why this is affecting my platform.
>> AFAIK there is no PCIe hardware on it AND I completely disabled PCIe
>> support in config.
>>
>> Kind regards,
>> Mike
This patch does not fix the problem, during boot the kernel still
panics. I had a closer look at the commit and the following patch
fixes it for me....
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 028ac1f..21b687f 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
if (ret)
goto err0;
} else {
- fsl_setup_indirect_pci(hose, rsrc_cfg.start,
+ setup_indirect_pci(hose, rsrc_cfg.start,
rsrc_cfg.start + 4, 0);
}
Apparently the mpc83xx platform code goes through great lengths to
guard the call to fsl_pcie_check_link on NON PCIe platforms, since
apparently this seems to cause the panic. Furthermore it also has its
own PCIe setup code. With this patch applied the system boots fine for
me.
The only problem I have with this patch is that the compiler fails
since -Werror is used and fsl_setup_indirect_pci now is never used.
For testing purposes I removed the function definition. Obviously the
proper fix would be to wrap it in an #ifdef but I did not know the
proper define to check against.
Kind regards,
Mike
On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> Good evening,
>
> On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <[email protected]>
> wrote:
> > Hi Mike,
> >
> > could you please try this patch:
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> > http://patchwork.ozlabs.org/patch/244515/
> >
> > Rojhalat
> >
> >
> > On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
> >> After bisecting I found the responsible commit.
> >>
> >> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
> >> PCIe controllers
> >>
> >> Reverting this commit allowed my board to boot again.
> >>
> >> @Rojhalat: Please have a look at
> >> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
> >> for my initial bugreport.
> >>
> >> What I do not understand at all is why this is affecting my
> platform.
> >> AFAIK there is no PCIe hardware on it AND I completely disabled
> PCIe
> >> support in config.
> >>
> >> Kind regards,
> >> Mike
>
> This patch does not fix the problem, during boot the kernel still
> panics. I had a closer look at the commit and the following patch
> fixes it for me....
>
> diff --git a/arch/powerpc/sysdev/fsl_pci.c
> b/arch/powerpc/sysdev/fsl_pci.c
> index 028ac1f..21b687f 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node
> *dev)
> if (ret)
> goto err0;
> } else {
> - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> + setup_indirect_pci(hose, rsrc_cfg.start,
> rsrc_cfg.start + 4, 0);
> }
The only difference here is that you're not setting hose->ops to
fsl_indirect_pci_ops. Do you know why that is helping, and what
hose->ops is set to instead?
-Scott
On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> > Good evening,
> >
> > On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <[email protected]>
> >
> > wrote:
> > > Hi Mike,
> > >
> > > could you please try this patch:
> > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> > > http://patchwork.ozlabs.org/patch/244515/
> > >
> > > Rojhalat
> > >
> > > On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
> > >> After bisecting I found the responsible commit.
> > >>
> > >> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
> > >> PCIe controllers
> > >>
> > >> Reverting this commit allowed my board to boot again.
> > >>
> > >> @Rojhalat: Please have a look at
> > >> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
> > >> for my initial bugreport.
> > >>
> > >> What I do not understand at all is why this is affecting my
> >
> > platform.
> >
> > >> AFAIK there is no PCIe hardware on it AND I completely disabled
> >
> > PCIe
> >
> > >> support in config.
> > >>
> > >> Kind regards,
> > >> Mike
> >
> > This patch does not fix the problem, during boot the kernel still
> > panics. I had a closer look at the commit and the following patch
> > fixes it for me....
> >
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> > b/arch/powerpc/sysdev/fsl_pci.c
> > index 028ac1f..21b687f 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node
> > *dev)
> >
> > if (ret)
> >
> > goto err0;
> >
> > } else {
> >
> > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> > + setup_indirect_pci(hose, rsrc_cfg.start,
> >
> > rsrc_cfg.start + 4, 0);
> >
> > }
>
> The only difference here is that you're not setting hose->ops to
> fsl_indirect_pci_ops. Do you know why that is helping, and what
> hose->ops is set to instead?
>
> -Scott
The difference is only the read function in hose->ops, which is set to
indirect_read_config instead of fsl_indirect_read_config.
fsl_indirect_read_config calls fsl_pcie_check_link, which is where the Oops
occurs.
Mike, can you find out where exactly in fsl_pcie_check_link the bad access
happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
Rojhalat
On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> > > Good evening,
> > >
> > > This patch does not fix the problem, during boot the kernel still
> > > panics. I had a closer look at the commit and the following patch
> > > fixes it for me....
> > >
> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> > > b/arch/powerpc/sysdev/fsl_pci.c
> > > index 028ac1f..21b687f 100644
> > > --- a/arch/powerpc/sysdev/fsl_pci.c
> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct
> device_node
> > > *dev)
> > >
> > > if (ret)
> > >
> > > goto err0;
> > >
> > > } else {
> > >
> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> > > + setup_indirect_pci(hose, rsrc_cfg.start,
> > >
> > > rsrc_cfg.start + 4, 0);
> > >
> > > }
> >
> > The only difference here is that you're not setting hose->ops to
> > fsl_indirect_pci_ops. Do you know why that is helping, and what
> > hose->ops is set to instead?
> >
> > -Scott
>
> The difference is only the read function in hose->ops, which is set to
> indirect_read_config instead of fsl_indirect_read_config.
>
> fsl_indirect_read_config calls fsl_pcie_check_link, which is where
> the Oops
> occurs.
Why is fsl_pcie_check_link being called for non-PCIe buses?
> Mike, can you find out where exactly in fsl_pcie_check_link the bad
> access
> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
Why does it matter? You shouldn't be calling that function at all.
-Scott
On Tue, Jun 11, 2013 at 7:00 PM, Scott Wood <[email protected]> wrote:
> On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
>>
>> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
>> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
>> > > Good evening,
>> > >
>> > > This patch does not fix the problem, during boot the kernel still
>> > > panics. I had a closer look at the commit and the following patch
>> > > fixes it for me....
>> > >
>> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
>> > > b/arch/powerpc/sysdev/fsl_pci.c
>> > > index 028ac1f..21b687f 100644
>> > > --- a/arch/powerpc/sysdev/fsl_pci.c
>> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
>> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node
>> > > *dev)
>> > >
>> > > if (ret)
>> > >
>> > > goto err0;
>> > >
>> > > } else {
>> > >
>> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
>> > > + setup_indirect_pci(hose, rsrc_cfg.start,
>> > >
>> > > rsrc_cfg.start + 4, 0);
>> > >
>> > > }
>> >
>> > The only difference here is that you're not setting hose->ops to
>> > fsl_indirect_pci_ops. Do you know why that is helping, and what
>> > hose->ops is set to instead?
>> >
>> > -Scott
>>
>> The difference is only the read function in hose->ops, which is set to
>> indirect_read_config instead of fsl_indirect_read_config.
>>
>> fsl_indirect_read_config calls fsl_pcie_check_link, which is where the
>> Oops
>> occurs.
>
>
> Why is fsl_pcie_check_link being called for non-PCIe buses?
>
>
>> Mike, can you find out where exactly in fsl_pcie_check_link the bad access
>> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
>
>
> Why does it matter? You shouldn't be calling that function at all.
>
> -Scott
For the record BUGVERBOSE is already set with this build so this is
the most detailed trace I get. And regarding Scott's remark, maybe I
was not clear enough in my first report. This is a PCI only board so I
also wondered about the call to fsl_pcie_check_link in the first
place. Since apparently the 83xx related add bridge code already has a
case for boards with PCIe support. So I think the change should really
happen somewhere in this code and not in the PCI only path.
/Mike
On 06/11/2013 12:09:42 PM, Michael Guntsche wrote:
> On Tue, Jun 11, 2013 at 7:00 PM, Scott Wood <[email protected]>
> wrote:
> > On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
> >>
> >> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> >> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> >> > > Good evening,
> >> > >
> >> > > This patch does not fix the problem, during boot the kernel
> still
> >> > > panics. I had a closer look at the commit and the following
> patch
> >> > > fixes it for me....
> >> > >
> >> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> >> > > b/arch/powerpc/sysdev/fsl_pci.c
> >> > > index 028ac1f..21b687f 100644
> >> > > --- a/arch/powerpc/sysdev/fsl_pci.c
> >> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
> >> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct
> device_node
> >> > > *dev)
> >> > >
> >> > > if (ret)
> >> > >
> >> > > goto err0;
> >> > >
> >> > > } else {
> >> > >
> >> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> >> > > + setup_indirect_pci(hose, rsrc_cfg.start,
> >> > >
> >> > > rsrc_cfg.start + 4, 0);
> >> > >
> >> > > }
> >> >
> >> > The only difference here is that you're not setting hose->ops to
> >> > fsl_indirect_pci_ops. Do you know why that is helping, and what
> >> > hose->ops is set to instead?
> >> >
> >> > -Scott
> >>
> >> The difference is only the read function in hose->ops, which is
> set to
> >> indirect_read_config instead of fsl_indirect_read_config.
> >>
> >> fsl_indirect_read_config calls fsl_pcie_check_link, which is where
> the
> >> Oops
> >> occurs.
> >
> >
> > Why is fsl_pcie_check_link being called for non-PCIe buses?
> >
> >
> >> Mike, can you find out where exactly in fsl_pcie_check_link the
> bad access
> >> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
> >
> >
> > Why does it matter? You shouldn't be calling that function at all.
> >
> > -Scott
>
> For the record BUGVERBOSE is already set with this build so this is
> the most detailed trace I get. And regarding Scott's remark, maybe I
> was not clear enough in my first report. This is a PCI only board so I
> also wondered about the call to fsl_pcie_check_link in the first
> place.
Yes, I figured it was non-PCIe because the code change that you said
helped was on the non-PCIe branch of the if/else. Generally it's good
to explicitly mention the chip you're using, though.
fsl_setup_indirect_pci should be renamed to fsl_setup_indirect_pcie.
Your patch above should be applied, and fsl_setup_indirect_pcie should
be moved into the booke/86xx ifdef to avoid an unused function warning.
-Scott
On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> On 06/11/2013 12:09:42 PM, Michael Guntsche wrote:
> > On Tue, Jun 11, 2013 at 7:00 PM, Scott Wood <[email protected]>
> >
> > wrote:
> > > On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
> > >> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> > >> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> > >> > > Good evening,
> > >> > >
> > >> > > This patch does not fix the problem, during boot the kernel
> >
> > still
> >
> > >> > > panics. I had a closer look at the commit and the following
> >
> > patch
> >
> > >> > > fixes it for me....
> > >> > >
> > >> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> > >> > > b/arch/powerpc/sysdev/fsl_pci.c
> > >> > > index 028ac1f..21b687f 100644
> > >> > > --- a/arch/powerpc/sysdev/fsl_pci.c
> > >> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > >> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct
> >
> > device_node
> >
> > >> > > *dev)
> > >> > >
> > >> > > if (ret)
> > >> > >
> > >> > > goto err0;
> > >> > >
> > >> > > } else {
> > >> > >
> > >> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> > >> > > + setup_indirect_pci(hose, rsrc_cfg.start,
> > >> > >
> > >> > > rsrc_cfg.start + 4, 0);
> > >> > >
> > >> > > }
> > >> >
> > >> > The only difference here is that you're not setting hose->ops to
> > >> > fsl_indirect_pci_ops. Do you know why that is helping, and what
> > >> > hose->ops is set to instead?
> > >> >
> > >> > -Scott
> > >>
> > >> The difference is only the read function in hose->ops, which is
> >
> > set to
> >
> > >> indirect_read_config instead of fsl_indirect_read_config.
> > >>
> > >> fsl_indirect_read_config calls fsl_pcie_check_link, which is where
> >
> > the
> >
> > >> Oops
> > >> occurs.
> > >
> > > Why is fsl_pcie_check_link being called for non-PCIe buses?
> > >
> > >> Mike, can you find out where exactly in fsl_pcie_check_link the
> >
> > bad access
> >
> > >> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
> > >
> > > Why does it matter? You shouldn't be calling that function at all.
> > >
> > > -Scott
> >
> > For the record BUGVERBOSE is already set with this build so this is
> > the most detailed trace I get. And regarding Scott's remark, maybe I
> > was not clear enough in my first report. This is a PCI only board so I
> > also wondered about the call to fsl_pcie_check_link in the first
> > place.
>
> Yes, I figured it was non-PCIe because the code change that you said
> helped was on the non-PCIe branch of the if/else. Generally it's good
> to explicitly mention the chip you're using, though.
>
> fsl_setup_indirect_pci should be renamed to fsl_setup_indirect_pcie.
> Your patch above should be applied, and fsl_setup_indirect_pcie should
> be moved into the booke/86xx ifdef to avoid an unused function warning.
>
> -Scott
How about this patch? It uses setup_indirect_pci for the PCI case in
mpc83xx_add_bridge. Additionally it adds a check in fsl_setup_indirect_pci
to only use the modified read function in case of PCIe.
Rojhalat
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 028ac1f..45670df 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -97,22 +97,23 @@ static int fsl_indirect_read_config(struct pci_bus *bus, unsigned int devfn,
return indirect_read_config(bus, devfn, offset, len, val);
}
-static struct pci_ops fsl_indirect_pci_ops =
+static struct pci_ops fsl_indirect_pcie_ops =
{
.read = fsl_indirect_read_config,
.write = indirect_write_config,
};
+#if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
+
static void __init fsl_setup_indirect_pci(struct pci_controller* hose,
resource_size_t cfg_addr,
resource_size_t cfg_data, u32 flags)
{
setup_indirect_pci(hose, cfg_addr, cfg_data, flags);
- hose->ops = &fsl_indirect_pci_ops;
+ if (early_find_capability(hose, 0, 0, PCI_CAP_ID_EXP)) /* PCIe */
+ hose->ops = &fsl_indirect_pcie_ops;
}
-#if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
-
#define MAX_PHYS_ADDR_BITS 40
static u64 pci64_dma_offset = 1ull << MAX_PHYS_ADDR_BITS;
@@ -814,8 +815,8 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
if (ret)
goto err0;
} else {
- fsl_setup_indirect_pci(hose, rsrc_cfg.start,
- rsrc_cfg.start + 4, 0);
+ setup_indirect_pci(hose, rsrc_cfg.start,
+ rsrc_cfg.start + 4, 0);
}
printk(KERN_INFO "Found FSL PCI host bridge at 0x%016llx. "
On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:
> On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> > Yes, I figured it was non-PCIe because the code change that you said
> > helped was on the non-PCIe branch of the if/else. Generally it's
> good
> > to explicitly mention the chip you're using, though.
> >
> > fsl_setup_indirect_pci should be renamed to fsl_setup_indirect_pcie.
> > Your patch above should be applied, and fsl_setup_indirect_pcie
> should
> > be moved into the booke/86xx ifdef to avoid an unused function
> warning.
> >
> > -Scott
>
> How about this patch? It uses setup_indirect_pci for the PCI case in
> mpc83xx_add_bridge. Additionally it adds a check in
> fsl_setup_indirect_pci
> to only use the modified read function in case of PCIe.
If we're adding the check to fsl_setup_indirect_pci, there's no need to
change the 83xx call back to setup_indirect_pci. I see that 85xx is
also callirng fsl_setup_indirect_pci for both; it'd be good to be
consistent.
In any case, can you send a proper patch with a signoff and commit
message?
-Scott
On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:
> On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:
> > On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> > > Yes, I figured it was non-PCIe because the code change that you said
> > > helped was on the non-PCIe branch of the if/else. Generally it's
> >
> > good
> >
> > > to explicitly mention the chip you're using, though.
> > >
> > > fsl_setup_indirect_pci should be renamed to fsl_setup_indirect_pcie.
> > > Your patch above should be applied, and fsl_setup_indirect_pcie
> >
> > should
> >
> > > be moved into the booke/86xx ifdef to avoid an unused function
> >
> > warning.
> >
> > > -Scott
> >
> > How about this patch? It uses setup_indirect_pci for the PCI case in
> > mpc83xx_add_bridge. Additionally it adds a check in
> > fsl_setup_indirect_pci
> > to only use the modified read function in case of PCIe.
>
> If we're adding the check to fsl_setup_indirect_pci, there's no need to
> change the 83xx call back to setup_indirect_pci. I see that 85xx is
> also callirng fsl_setup_indirect_pci for both; it'd be good to be
> consistent.
>
> In any case, can you send a proper patch with a signoff and commit
> message?
>
> -Scott
Where is it called for 85xx? As far as I can tell fsl_setup_indirect_pci is
called exactly once in fsl_add_bridge and nowhere else (after applying the
proposed patch).
For 83xx the decision between PCI and PCIe has already been made at
the point where the setup function is called. So IMO it doesn't make sense
to call fsl_setup_indirect_pci and do the check again. Moreover PCIe on 83xx
uses a completely different set of functions.
I'll send the proper patch in a separate mail.
Rojhalat
On 06/13/2013 02:21:24 AM, Rojhalat Ibrahim wrote:
> On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:
> > On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:
> > > On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> > > > Yes, I figured it was non-PCIe because the code change that you
> said
> > > > helped was on the non-PCIe branch of the if/else. Generally
> it's
> > >
> > > good
> > >
> > > > to explicitly mention the chip you're using, though.
> > > >
> > > > fsl_setup_indirect_pci should be renamed to
> fsl_setup_indirect_pcie.
> > > > Your patch above should be applied, and fsl_setup_indirect_pcie
> > >
> > > should
> > >
> > > > be moved into the booke/86xx ifdef to avoid an unused function
> > >
> > > warning.
> > >
> > > > -Scott
> > >
> > > How about this patch? It uses setup_indirect_pci for the PCI case
> in
> > > mpc83xx_add_bridge. Additionally it adds a check in
> > > fsl_setup_indirect_pci
> > > to only use the modified read function in case of PCIe.
> >
> > If we're adding the check to fsl_setup_indirect_pci, there's no
> need to
> > change the 83xx call back to setup_indirect_pci. I see that 85xx is
> > also callirng fsl_setup_indirect_pci for both; it'd be good to be
> > consistent.
> >
> > In any case, can you send a proper patch with a signoff and commit
> > message?
> >
> > -Scott
>
> Where is it called for 85xx? As far as I can tell
> fsl_setup_indirect_pci is
> called exactly once in fsl_add_bridge and nowhere else (after
> applying the
> proposed patch).
fsl_add_bridge() is where it's called for 85xx.
> For 83xx the decision between PCI and PCIe has already been made at
> the point where the setup function is called. So IMO it doesn't make
> sense
> to call fsl_setup_indirect_pci and do the check again. Moreover PCIe
> on 83xx
> uses a completely different set of functions.
My concern is consistency. E.g. if 85xx is using
fsl_setup_indirect_pci for both, but 83xx isn't, then a developer using
83xx could end up breaking 85xx by introducing another PCIe dependency
in fsl_setup_indirect_pci. Or an 85xx developer could put something
non-PCIe-related in fsl_setup_indirect_pci that 83xx would benefit from.
Alternatively, you could call it fsl_setup_indirect_pcie, and move the
PCIe check into fsl_add_bridge().
-Scott
On Thursday 13 June 2013 11:49:17 Scott Wood wrote:
> On 06/13/2013 02:21:24 AM, Rojhalat Ibrahim wrote:
> > On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:
> > > On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:
> > > > On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> > > > > Yes, I figured it was non-PCIe because the code change that you
> >
> > said
> >
> > > > > helped was on the non-PCIe branch of the if/else. Generally
> >
> > it's
> >
> > > > good
> > > >
> > > > > to explicitly mention the chip you're using, though.
> > > > >
> > > > > fsl_setup_indirect_pci should be renamed to
> >
> > fsl_setup_indirect_pcie.
> >
> > > > > Your patch above should be applied, and fsl_setup_indirect_pcie
> > > >
> > > > should
> > > >
> > > > > be moved into the booke/86xx ifdef to avoid an unused function
> > > >
> > > > warning.
> > > >
> > > > > -Scott
> > > >
> > > > How about this patch? It uses setup_indirect_pci for the PCI case
> >
> > in
> >
> > > > mpc83xx_add_bridge. Additionally it adds a check in
> > > > fsl_setup_indirect_pci
> > > > to only use the modified read function in case of PCIe.
> > >
> > > If we're adding the check to fsl_setup_indirect_pci, there's no
> >
> > need to
> >
> > > change the 83xx call back to setup_indirect_pci. I see that 85xx is
> > > also callirng fsl_setup_indirect_pci for both; it'd be good to be
> > > consistent.
> > >
> > > In any case, can you send a proper patch with a signoff and commit
> > > message?
> > >
> > > -Scott
> >
> > Where is it called for 85xx? As far as I can tell
> > fsl_setup_indirect_pci is
> > called exactly once in fsl_add_bridge and nowhere else (after
> > applying the
> > proposed patch).
>
> fsl_add_bridge() is where it's called for 85xx.
>
> > For 83xx the decision between PCI and PCIe has already been made at
> > the point where the setup function is called. So IMO it doesn't make
> > sense
> > to call fsl_setup_indirect_pci and do the check again. Moreover PCIe
> > on 83xx
> > uses a completely different set of functions.
>
> My concern is consistency. E.g. if 85xx is using
> fsl_setup_indirect_pci for both, but 83xx isn't, then a developer using
> 83xx could end up breaking 85xx by introducing another PCIe dependency
> in fsl_setup_indirect_pci. Or an 85xx developer could put something
> non-PCIe-related in fsl_setup_indirect_pci that 83xx would benefit from.
>
> Alternatively, you could call it fsl_setup_indirect_pcie, and move the
> PCIe check into fsl_add_bridge().
>
> -Scott
Ok. I'll post a v2 of the patch.
Rojhalat