[PATCH] enable PCI bridges in MIPS ip32

[PATCH] enable PCI bridges in MIPS ip32

[Giuseppe Sacco]

Use bus numbering when addressing device with MACE chipset
in order to support PCI bridges.
Changes in chkslot() and mkaddr() #defines.

Signed-off-by: Giuseppe Sacco <eppesuig@debian.org>
---

Hi Ralf,
I managed to create a patch against current 2.6.23-rc9 git tree
for supporting PCI bridges on SGI ip32 machines.
This is my first kernel patch, so I am usure about the correct way
to send a patch. Please let me know if anything is wrong.

Bye,
Giuseppe

diff --git a/arch/mips/pci/ops-mace.c b/arch/mips/pci/ops-mace.c
index 8008e31..18a7159 100644
--- a/arch/mips/pci/ops-mace.c
+++ b/arch/mips/pci/ops-mace.c
@@ -31,20 +31,21 @@
 
 #define chkslot(_bus,_devfn)					\
 do {							        \
-	if ((_bus)->number > 0 || PCI_SLOT (_devfn) < 1	\
-	    || PCI_SLOT (_devfn) > 3)			        \
+	if ((_bus)->number > 1 ||                               \
+		((_bus)->number == 0 && (PCI_SLOT (_devfn) < 1  \
+	    	|| PCI_SLOT (_devfn) > 3)))		        \
 		return PCIBIOS_DEVICE_NOT_FOUND;		\
 } while (0)
 
-#define mkaddr(_devfn, _reg) \
-((((_devfn) & 0xffUL) << 8) | ((_reg) & 0xfcUL))
+#define mkaddr(_bus, _devfn, _reg) \
+((((_bus)->number & 0xffUL) << 16) | (((_devfn) & 0xffUL) << 8) | ((_reg) & 0xfcUL))
 
 static int
 mace_pci_read_config(struct pci_bus *bus, unsigned int devfn,
 		     int reg, int size, u32 *val)
 {
 	chkslot(bus, devfn);
-	mace->pci.config_addr = mkaddr(devfn, reg);
+	mace->pci.config_addr = mkaddr(bus, devfn, reg);
 	switch (size) {
 	case 1:
 		*val = mace->pci.config_data.b[(reg & 3) ^ 3];
@@ -67,7 +68,7 @@ mace_pci_write_config(struct pci_bus *bus, unsigned int devfn,
 		      int reg, int size, u32 val)
 {
 	chkslot(bus, devfn);
-	mace->pci.config_addr = mkaddr(devfn, reg);
+	mace->pci.config_addr = mkaddr(bus, devfn, reg);
 	switch (size) {
 	case 1:
 		mace->pci.config_data.b[(reg & 3) ^ 3] = val;

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Giuseppe Sacco wrote:

> I managed to create a patch against current 2.6.23-rc9 git tree
> for supporting PCI bridges on SGI ip32 machines.
> This is my first kernel patch, so I am usure about the correct way
> to send a patch. Please let me know if anything is wrong.

 I am glad you have succeeded.  A couple of minor notes below.

> @@ -31,20 +31,21 @@
>  
>  #define chkslot(_bus,_devfn)					\
>  do {							        \
> -	if ((_bus)->number > 0 || PCI_SLOT (_devfn) < 1	\
> -	    || PCI_SLOT (_devfn) > 3)			        \
> +	if ((_bus)->number > 1 ||                               \
> +		((_bus)->number == 0 && (PCI_SLOT (_devfn) < 1  \
> +	    	|| PCI_SLOT (_devfn) > 3)))		        \
>  		return PCIBIOS_DEVICE_NOT_FOUND;		\

 I think you should allow any bus numbers, not only 0 and 1 -- while 
possibly unlikely, you may have a tree of bridges on an option card.  The 
generic code should handle it fine -- you need not care.

> -#define mkaddr(_devfn, _reg) \
> -((((_devfn) & 0xffUL) << 8) | ((_reg) & 0xfcUL))
> +#define mkaddr(_bus, _devfn, _reg) \
> +((((_bus)->number & 0xffUL) << 16) | (((_devfn) & 0xffUL) << 8) | ((_reg) & 0xfcUL))

 Please fit your lines in 80 characters.

> -	mace->pci.config_addr = mkaddr(devfn, reg);
> +	mace->pci.config_addr = mkaddr(bus, devfn, reg);

 It may be more consistent if you pass just "bus->number".  You may neatly 
avoid the line wrap above this way too.

 Have you run your change through `scripts/checkpatch.pl'?

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Giuseppe Sacco]

Hi Maciej,

Il giorno gio, 04/10/2007 alle 13.27 +0100, Maciej W. Rozycki ha
scritto:
[...]
>  It may be more consistent if you pass just "bus->number".  You may neatly 
> avoid the line wrap above this way too.
> 
>  Have you run your change through `scripts/checkpatch.pl'?

I'll provide a new patch tomorrow.

Thanks,
Giuseppe

Re: [PATCH] enable PCI bridges in MIPS ip32

[Ralf Baechle]

On Thu, Oct 04, 2007 at 01:27:47PM +0100, Maciej W. Rozycki wrote:

> On Thu, 4 Oct 2007, Giuseppe Sacco wrote:
> 
> > I managed to create a patch against current 2.6.23-rc9 git tree
> > for supporting PCI bridges on SGI ip32 machines.
> > This is my first kernel patch, so I am usure about the correct way
> > to send a patch. Please let me know if anything is wrong.
> 
>  I am glad you have succeeded.  A couple of minor notes below.
> 
> > @@ -31,20 +31,21 @@
> >  
> >  #define chkslot(_bus,_devfn)					\
> >  do {							        \
> > -	if ((_bus)->number > 0 || PCI_SLOT (_devfn) < 1	\
> > -	    || PCI_SLOT (_devfn) > 3)			        \
> > +	if ((_bus)->number > 1 ||                               \
> > +		((_bus)->number == 0 && (PCI_SLOT (_devfn) < 1  \
> > +	    	|| PCI_SLOT (_devfn) > 3)))		        \
> >  		return PCIBIOS_DEVICE_NOT_FOUND;		\
> 
>  I think you should allow any bus numbers, not only 0 and 1 -- while 
> possibly unlikely, you may have a tree of bridges on an option card.  The 
> generic code should handle it fine -- you need not care.

I think historically we had something like chkslot() first in the code
for the Galileo/Marvell bridges where it's needed due the brainddead
abuse of device 31 - any access to that will crash the system.  From that
point on chkslot checking spread across the PCI code like the measles in
a kindergarden.

Another stylistic comment is about the return statement in the macro.
That sort of construct should be avoided, it's quite unobvious to the
reader who isn't familiar with the macro definition.

Another thing the historically extremly widespread use of macros in the
Linux kernel.  Macros tend to be harder to read and maintain but
historically gcc was doing a rather bad job at optimizing inline functions
because inlining happend rather late in the compilation process when many
of the optimizations already had been performed.  That is no longer true
for modern gcc so these days functions are prefered.  So adding this all
up:

static inline int chkslot(struct pci_bus *bus, unsigned int devfn)
{
	return bus->number > 1 ||
	       (bus->number == 0 && (PCI_SLOT(devfn) < 1 ||
		PCI_SLOT(devfn) > 3));
}

static inline int mkaddr(struct pci_bus *bus, unsigned int devfn,
	unsigned int reg)
{
	return ((bus->number & 0xff) << 16) |
	       ((devfn & 0xff) << 8) |
	       (reg & 0xfc);
}

static int
mace_pci_read_config(struct pci_bus *bus, unsigned int devfn,
                     int reg, int size, u32 *val)
{
	if (chkslot(bus, devfn)
		return PCIBIOS_DEVICE_NOT_FOUND;

	mace->pci.config_addr = mkaddr(bus, devfn, reg);
[...]

(of course this all goes far beyond Giuseppe's patch - but the whole
ops-mace.c file like so much of the other code in arch/mips/pci isn't
exactly an example to be copied.

Ah, one final formality - when sending a patch please add a
Signed-off-by: line.  See Documentation/SubmittingPatches in the kernel
tree for what this means.

So Giuseppe, I'm happy to apply your patch because it makes sense and
doesn't add new badness.

Cheers,

  Ralf

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Ralf Baechle wrote:

> I think historically we had something like chkslot() first in the code
> for the Galileo/Marvell bridges where it's needed due the brainddead
> abuse of device 31 - any access to that will crash the system.  From that
> point on chkslot checking spread across the PCI code like the measles in
> a kindergarden.

 Does the Galileo/Marvell do anything else with the device #31 than what 
is recommended by the PCI spec as a way to issue special cycles?  We need 
to be careful about the device #31 in general; it is seldom used anyway as 
there are only 20 IDSEL lines defined by the standard and they are usually 
mapped starting from the device #0.

> Another stylistic comment is about the return statement in the macro.
> That sort of construct should be avoided, it's quite unobvious to the
> reader who isn't familiar with the macro definition.

[etc... about macros]

 Very true indeed -- I sort of slipped over the issue unconsciously as it 
is outside the scope of the fix itself.  It is usually a good idea to 
separate clean-ups from changes to the semantics.

> Ah, one final formality - when sending a patch please add a
> Signed-off-by: line.  See Documentation/SubmittingPatches in the kernel
> tree for what this means.

 Well, there was one actually in the submission. :-)  And `checkpatch.pl' 
would remind about it otherwise.

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Ralf Baechle]

On Thu, Oct 04, 2007 at 03:13:01PM +0100, Maciej W. Rozycki wrote:

> > I think historically we had something like chkslot() first in the code
> > for the Galileo/Marvell bridges where it's needed due the brainddead
> > abuse of device 31 - any access to that will crash the system.  From that
> > point on chkslot checking spread across the PCI code like the measles in
> > a kindergarden.
> 
>  Does the Galileo/Marvell do anything else with the device #31 than what 
> is recommended by the PCI spec as a way to issue special cycles?  We need 
> to be careful about the device #31 in general; it is seldom used anyway as 
> there are only 20 IDSEL lines defined by the standard and they are usually 
> mapped starting from the device #0.

It's documented somewhere in their specs.  Whatever, it ends crashing
the system so device 31 is hands off.

  Ralf

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Ralf Baechle wrote:

> It's documented somewhere in their specs.  Whatever, it ends crashing
> the system so device 31 is hands off.

 OK, found it -- it is the GT-64120A erratum FEr#19 leading to a hang of 
the device; perhaps we should mention it briefly in the source code.

 While the PCI spec says reads from the device #31, function #7 for host 
bridges implementing the recommended special cycle generation mechanism 
have undefined results, this behaviour is certainly "undefined" in a very 
silly way and then, according to the spec, it must not happen for the 
function #0, which is the only one probed by Linux by default for 
single-function devices.

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Maciej W. Rozycki wrote:

> to be careful about the device #31 in general; it is seldom used anyway as 
> there are only 20 IDSEL lines defined by the standard and they are usually 
> mapped starting from the device #0.

 Just to correct myself not to confuse anybody -- there are actually 21 
IDSEL lines which map from bits 31:11 of the address provided for Type 0 
configuration access cycles -- at most one bit from that range is ever set 
for such cycles.  The issuing bridge is free to set no bits for device 
numbers it has no assigned IDSEL line for; such transactions will never be 
claimed by any device and thus will always terminate with Master-Abort.

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Giuseppe Sacco]

Il giorno gio, 04/10/2007 alle 14.03 +0100, Ralf Baechle ha scritto:
[...]
> (of course this all goes far beyond Giuseppe's patch - but the whole
> ops-mace.c file like so much of the other code in arch/mips/pci isn't
> exactly an example to be copied.
> 
> Ah, one final formality - when sending a patch please add a
> Signed-off-by: line.  See Documentation/SubmittingPatches in the kernel
> tree for what this means.

I'll provide a new patch tomorrow, using inline functions instead of
macros. About the maximum number of PCI buses, I used 1 since the ip32
only have 1 slot. If this maximum value should be changed, please let me
know.

Bye,
Giuseppe

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Giuseppe Sacco wrote:

> I'll provide a new patch tomorrow, using inline functions instead of
> macros. About the maximum number of PCI buses, I used 1 since the ip32
> only have 1 slot. If this maximum value should be changed, please let me
> know.

 You can have more than one bridge on a card.  Or if you have e.g. a 
PCI-HyperTransport bridge on a PCI card, then who knows what may lie 
beyond.  In any case there is no need to do this check here and it is even 
harmful in the sense it just bumps the unnecessary limitation by one 
rather than removing it altogether -- the generic PCI code that we have in 
drivers/pci/ will scan the bus behind the bridge and find out whether 
there are any others and act accordingly.

 In general you need not do any range checking here, even for the root 
bus, unless the host bridge is broken somehow and produces unexpected 
behaviour when an inexistent device is accessed with a configuration 
cycle.  Normally such a transaction should result with a Master Abort and 
be handled gracefully by the originating host bridge.  This is not always 
the case, sometimes because of a flaw in hardware and sometimes because of 
misconfiguration -- unfortunately the quality of bootstrap firmware varies 
and fixing it up requires bridge-specific knowledge, which we sometimes 
have and use (grep for MSC01_PCI_CFG_MAXRTRY_MSK for an example), but 
sometimes we do not.

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Ralf Baechle]

On Thu, Oct 04, 2007 at 04:33:33PM +0200, Giuseppe Sacco wrote:

> Il giorno gio, 04/10/2007 alle 14.03 +0100, Ralf Baechle ha scritto:
> [...]
> > (of course this all goes far beyond Giuseppe's patch - but the whole
> > ops-mace.c file like so much of the other code in arch/mips/pci isn't
> > exactly an example to be copied.
> > 
> > Ah, one final formality - when sending a patch please add a
> > Signed-off-by: line.  See Documentation/SubmittingPatches in the kernel
> > tree for what this means.
> 
> I'll provide a new patch tomorrow, using inline functions instead of
> macros. About the maximum number of PCI buses, I used 1 since the ip32
> only have 1 slot. If this maximum value should be changed, please let me
> know.

The entire testing done by chkslot() is probably not needed, so I suggest
you try to simply dump the thing entirely and test.

  Ralf

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Thu, 4 Oct 2007, Ralf Baechle wrote:

> The entire testing done by chkslot() is probably not needed, so I suggest
> you try to simply dump the thing entirely and test.

 Exactly what I wrote too. :-)  Though I would imagine it was introduced 
for a reason, like a bug in the host bridge or something, as already 
suggested.  Otherwise what would the point have been?

  Maciej

Re: [PATCH] enable PCI bridges in MIPS ip32

[Ralf Baechle]

On Thu, Oct 04, 2007 at 04:27:57PM +0100, Maciej W. Rozycki wrote:

> On Thu, 4 Oct 2007, Ralf Baechle wrote:
> 
> > The entire testing done by chkslot() is probably not needed, so I suggest
> > you try to simply dump the thing entirely and test.
> 
>  Exactly what I wrote too. :-)  Though I would imagine it was introduced 
> for a reason, like a bug in the host bridge or something, as already 
> suggested.  Otherwise what would the point have been?

I suspect cut-and-paste-o-mania, probably originally started by the
necessity of doing so for the Galileo chips.

  Ralf

Re: [PATCH] enable PCI bridges in MIPS ip32

[Giuseppe Sacco]

On Thu, 4 Oct 2007 16:32:17 +0100 Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Oct 04, 2007 at 04:27:57PM +0100, Maciej W. Rozycki wrote:
> > On Thu, 4 Oct 2007, Ralf Baechle wrote:
> > > The entire testing done by chkslot() is probably not needed, so I suggest
> > > you try to simply dump the thing entirely and test.
> > 
> >  Exactly what I wrote too. :-)  Though I would imagine it was introduced 
> > for a reason, like a bug in the host bridge or something, as already 
> > suggested.  Otherwise what would the point have been?
> 
> I suspect cut-and-paste-o-mania, probably originally started by the
> necessity of doing so for the Galileo chips.

After removing the chkslot() call, I get these errors when booting:
[...]
SCSI subsystem initialized
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00008000 (C)
MACEPCI: Master abort at 0x00010000 (C)
MACEPCI: Master abort at 0x00020000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
MACEPCI: Master abort at 0x00000000 (C)
PCI: Bridge: 0000:00:03.0
  IO window: 1000-1fff
  MEM window: 80000000-800fffff
  PREFETCH window: 80100000-801fffff
PCI: Enabling device 0000:00:03.0 (0000 -> 0003)
[...]

It seems all probes to devfn=0 fails. There is even a call on bus=2, that I really don't understand. the current lspci output is:

00:01.0 SCSI storage controller: Adaptec AIC-7880U
00:02.0 SCSI storage controller: Adaptec AIC-7880U
00:03.0 PCI bridge: NetMos Technology Unknown device 9250 (rev 01)
01:08.0 USB Controller: NEC Corporation USB (rev 43)
01:08.1 USB Controller: NEC Corporation USB (rev 43)
01:08.2 USB Controller: NEC Corporation USB 2.0 (rev 04)
01:09.0 FireWire (IEEE 1394): Agere Systems FW323 (rev 61)
01:0a.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8169 Gigabit Ethernet (rev 10)

So probably, the test was correct.  Should I restore the same check or only check for devfn==0?

Bye,
Giueppe

Re: [PATCH] enable PCI bridges in MIPS ip32

[Maciej W. Rozycki]

On Fri, 5 Oct 2007, Giuseppe Sacco wrote:

> After removing the chkslot() call, I get these errors when booting:
> [...]
> SCSI subsystem initialized
> MACEPCI: Master abort at 0x00000000 (C)
> MACEPCI: Master abort at 0x00008000 (C)
> MACEPCI: Master abort at 0x00010000 (C)
[...]

 Well, these are not errors in this context even though they seem to be 
reported as such -- these Master Aborts are expected to happen for non 
occupied slots (device numbers).  And reporting them to the user in this 
context seems silly (unless debugging).

 I can see the are coming from the MACE error interrupt handler -- either 
the Master-Abort interrupt should be masked for the duration of the 
configuration space access or, if impossible or the way to do so is 
unknown, the handler should recognise the context and silently ack the 
interrupt and pass the error up somehow.  It is up to code handling the 
host bridge in question to get it right -- see 
arch/mips/pci/ops-bonito64.c for an example.

> PCI: Bridge: 0000:00:03.0
>   IO window: 1000-1fff
>   MEM window: 80000000-800fffff
>   PREFETCH window: 80100000-801fffff
> PCI: Enabling device 0000:00:03.0 (0000 -> 0003)
> [...]
> 
> It seems all probes to devfn=0 fails. There is even a call on bus=2, 
> that I really don't understand. the current lspci output is:

 Well, perhaps the initial setup of the PCI-to-PCI bridge reports the 
subordinate bus to be 2 or something.  If you post the whole PCI probe 
log, someone may be able to provide an explanation.

 And devfn=0 failing probably means the host bridge does not want to 
report itself in the PCI configuration space; that is a valid approach 
(seen with the Alphas too, for example), although personally I do not like 
it very much.

> So probably, the test was correct.  Should I restore the same check or 
> only check for devfn==0?

 The handling of Master Aborts should be fixed instead.  It looks like 
MACEPCI_CONTROL_MAR_INT might be the right mask bit -- do we have a spec 
for the chip anywhere or is <asm-mips/ip32/mace.h> the only source?

  Maciej