We have redone the I/O layer in the Altix code.
We've broken the patch set down to 2 patches. One to remove the files,
the other to add in the new code. Most of the changes from the last
posting are in response to review comments.
Signed-off-by: Patrick Gefre <[email protected]>
The patches are :
ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
ftp://oss.sgi.com/projects/sn2/sn2-update/002-add-files
They are based off http://lia64.bkbits.net/linux-ia64-release-2.6.9
The general differences between the new code and the old code are:
I/O discovery and initialization was moved to prom to enable us to move
towards EFI 1.10 and ACPI compliance. EFI 1.10 and ACPI compliance
will be the next 2 phases in our development. Since prom is now
performing all I/O discovery and initialization, we had to re-architect
the Altix platform specific code in Linux - basically deleting all code
related to discovery and initialization and leaving DMA mapping which
was rewritten.
Until we can implement ACPI in our prom, we will use platform specific
SAL calls to retrieve any PCI configuration that is needed during the
PCI fixup phase.
Note that this new code requires a new Altix prom. If you need one, you
can email me and I can set you up with the proper people to get one.
Also we did not break out the pci_dma.c code (as Christoph has
suggested) - we are in the process of doing that and will submit that
code change in the near future.
--
Patrick Gefre
Silicon Graphics, Inc. (E-Mail) [email protected]
2750 Blue Water Rd (Voice) (651) 683-3127
Eagan, MN 55121-1400 (FAX) (651) 683-3054
I'm ok with the delete/add of most of the SGI
specific files (maybe it still isn't perfect yet,
but it may be close enough to take it, and then
clean up with some small patches).
But you seem to be touching some files outside of pure SGI
stuff. These two are a bit of a concern:
include/asm-ia64/io.h
arch/ia64/pci/pci.c
These others are outside of my area (well I *might* push
the drivers that are only used by SGI ... but hotplug
and qla1280 are definitely not mine). So they need to be
split out into separate patches.
drivers/char/mmtimer.c
drivers/char/snsc.c
drivers/ide/pci/sgiioc4.c
drivers/pci/hotplug/Kconfig
drivers/scsi/qla1280.c
drivers/serial/sn_console.c
-Tony
On Monday, October 4, 2004 10:13 pm, Luck, Tony wrote:
> I'm ok with the delete/add of most of the SGI
> specific files (maybe it still isn't perfect yet,
> but it may be close enough to take it, and then
> clean up with some small patches).
>
> But you seem to be touching some files outside of pure SGI
> stuff. These two are a bit of a concern:
>
> include/asm-ia64/io.h
Not sure about these changes...
> arch/ia64/pci/pci.c
It looks like the only non-codingstyle change here is to make pci_root_ops
non-static (and btw, some of the CodingStyle fixups look wrong). If it needs
to be non-static, it should be declared in a header file so we don't have to
extern it in sn_pci_fixup_bus.
Jesse
Also is there any chance you can move the additional rrb allocation for
the qlogic adapters into the prom so the driver doesn't have to bother
with it? That way the whole SN_SAL_IOIF_RRB_ALLOC could go away.
On Tue, Oct 05, 2004 at 08:43:44AM -0700, Jesse Barnes wrote:
...
> > arch/ia64/pci/pci.c
>
> It looks like the only non-codingstyle change here is to make pci_root_ops
> non-static (and btw, some of the CodingStyle fixups look wrong). If it needs
> to be non-static, it should be declared in a header file so we don't have to
> extern it in sn_pci_fixup_bus.
pci_root_ops should be static. It's only intended for ACPI.
Maybe rename pci_root_ops to "acpi_pci_ops" would make that clearer.
If SN2 platform needs hacks, then define "sn2_acpi_pci_ops" someplace
in the SN2 specific source code. In this case, SN2 will also
need to continue to NOT use pci_acpi_scan_root() and define it's
own discovery. When SN2 firmware can support pci_acpi_scan_root(),
then it would make sense to drop the SN2 specific PCI discovery and pci_ops.
And both can co-exist - SN2 code can check which version of firmware
is installed and invoke ia64 ACPI support if that is known to work.
If SN2 needs something defined in the ACPI spec but missing from ia64
ACPI support, add the missing bits to arch/ia64/pci/pci.c.
It just shouldn't interfere with current use.
hth,
grant
On Mon, Oct 04, 2004 at 04:57:06PM -0500, Pat Gefre wrote:
>
> We have redone the I/O layer in the Altix code.
>
> We've broken the patch set down to 2 patches. One to remove the files,
> the other to add in the new code. Most of the changes from the last
> posting are in response to review comments.
This looks pretty nice already, but a few small but important issues
need sorting out.
- The interface between pci_dma.c and the lowlevel code is still wrong -
and because of the additional 32bit direct translation in this code drop
it got even worse because you might be calling into a function just to
error out again.
Please implement my suggestions from month ago, it's not a lot of work.
- various sall baclls take bus_number and devfs but no pci domain, while
the normal SAL calls do, I think you should make the kernel code aware
of pci domains so the prom can introduce them seamlessly
- is doing SAL calls from irq context really safe? Also why do you need
different SAL calls for shub vs ice error? The prom should be easily
able to find out what hub a given nasid corresponds to.
- the patch reformats various unrelated or only slightly related files.
Please don't do that - in general the new style is better than the old
one, but it doesn't belong in this patchA
- there's a SNDRV_SHUB_GET_IOCTL_VERSION ioctl define added but never
used. In fact it looks like all SNDRV_SHUB_ values are unused now?
On Tue, Oct 05, 2004 at 09:22:01AM -0700, Grant Grundler wrote:
> pci_root_ops should be static. It's only intended for ACPI.
What I had intended when I wrote this code was that platforms that didn't
want to use the generic SAL code (and why not? It doesn't seem like it
should be the hardest thing in the world to move your hacks into SAL)
was that people should override
struct pci_raw_ops *raw_pci_ops = &pci_sal_ops;
by just assigning raw_pci_ops in their own code. I haven't looked at
the SGI code yet, but this is how arch/i386/pci/direct.c (for example)
works.
> Maybe rename pci_root_ops to "acpi_pci_ops" would make that clearer.
No. Don't rename it to anything ACPI specific. It isn't. It's just an
alternative route to access configuration space when you don't even
have a PCI bus, let alone a device.
--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain
Luck, Tony wrote:
> I'm ok with the delete/add of most of the SGI
> specific files (maybe it still isn't perfect yet,
> but it may be close enough to take it, and then
> clean up with some small patches).
>
> But you seem to be touching some files outside of pure SGI
> stuff. These two are a bit of a concern:
>
> include/asm-ia64/io.h
> arch/ia64/pci/pci.c
>
Most of this is Lindent changes. The only mod is we need pci_root_ops to be non-static.
> These others are outside of my area (well I *might* push
> the drivers that are only used by SGI ... but hotplug
> and qla1280 are definitely not mine). So they need to be
> split out into separate patches.
>
As a general comment, the changes to these files are because of mods in the reorg code - so they are
needed for this base but not in the older code. So, in my mind, it is a package - or should be taken
as a whole. I can break them out, but I think they need to go together.
> drivers/char/mmtimer.c
This is Jesse's code. We made an include file change. Is this OK Jesse ?
> drivers/char/snsc.c
This is Greg Howard's code - he's reviewed and approved the mod. Should I add him as a signed-off ?
> drivers/ide/pci/sgiioc4.c
More Lindent mods. We took out the endian code - not needed anymore.
> drivers/pci/hotplug/Kconfig
Took out SGI PCI Hotplug. Since there isn't any code behind it - we will add it back in when we
submit the code for it.
> drivers/scsi/qla1280.c
Again more Lindent mods. Took out the vchan definition hack.
> drivers/serial/sn_console.c
>
This is my driver.
Christoph Hellwig wrote:
> On Mon, Oct 04, 2004 at 04:57:06PM -0500, Pat Gefre wrote:
>
>>We have redone the I/O layer in the Altix code.
>>
>>We've broken the patch set down to 2 patches. One to remove the files,
>>the other to add in the new code. Most of the changes from the last
>>posting are in response to review comments.
>
>
> This looks pretty nice already, but a few small but important issues
> need sorting out.
> - the patch reformats various unrelated or only slightly related files.
> Please don't do that - in general the new style is better than the old
> one, but it doesn't belong in this patchA
Guess I don't understand this. Either the code base is Lindent'd or it isn't.
On Tuesday, October 5, 2004 11:20 am, Patrick Gefre wrote:
> > These others are outside of my area (well I *might* push
> > the drivers that are only used by SGI ... but hotplug
> > and qla1280 are definitely not mine). So they need to be
> > split out into separate patches.
>
> As a general comment, the changes to these files are because of mods in the
> reorg code - so they are needed for this base but not in the older code.
> So, in my mind, it is a package - or should be taken as a whole. I can
> break them out, but I think they need to go together.
Yeah, that's fine, but it's easier to review and integrate if patches to
update the API are separate files and mails (e.g. 2/3, 3/3, etc.). Makes for
more detailed changelog comments and makes changes in the bk tree much easier
to track. In general, the smaller, the better.
> > drivers/char/mmtimer.c
>
> This is Jesse's code. We made an include file change. Is this OK Jesse ?
Yeah, that's fine.
> > drivers/ide/pci/sgiioc4.c
>
> More Lindent mods. We took out the endian code - not needed anymore.
Sounds like that could be a separate cleanup patch.
> > drivers/pci/hotplug/Kconfig
>
> Took out SGI PCI Hotplug. Since there isn't any code behind it - we will
> add it back in when we submit the code for it.
Sounds good, probably a separate patch too.
Thanks,
Jesse
Matthew Wilcox wrote:
>On Tue, Oct 05, 2004 at 09:22:01AM -0700, Grant Grundler wrote:
>
>
>>pci_root_ops should be static. It's only intended for ACPI.
>>
>>
>
>What I had intended when I wrote this code was that platforms that didn't
>want to use the generic SAL code (and why not? It doesn't seem like it
>should be the hardest thing in the world to move your hacks into SAL)
>was that people should override
>
> struct pci_raw_ops *raw_pci_ops = &pci_sal_ops;
>
>by just assigning raw_pci_ops in their own code. I haven't looked at
>the SGI code yet, but this is how arch/i386/pci/direct.c (for example)
>works.
>
Hi Matthew,
Yes, after looking at Grant's review/suggestion, we found that we can
actually just use raw_pci_ops. This will work well for us. We have
incoorporated this change. No changes in pci/pci.c needed.
Thanks you for your information.
Thanks.
colin
>
>
>
>>Maybe rename pci_root_ops to "acpi_pci_ops" would make that clearer.
>>
>>
>
>No. Don't rename it to anything ACPI specific. It isn't. It's just an
>alternative route to access configuration space when you don't even
>have a PCI bus, let alone a device.
>
>
>
On Tue, Oct 05, 2004 at 06:45:58PM +0100, Matthew Wilcox wrote:
> On Tue, Oct 05, 2004 at 09:22:01AM -0700, Grant Grundler wrote:
> > pci_root_ops should be static. It's only intended for ACPI.
>
> What I had intended when I wrote this code was that platforms that didn't
> want to use the generic SAL code (and why not? It doesn't seem like it
> should be the hardest thing in the world to move your hacks into SAL)
> was that people should override
>
> struct pci_raw_ops *raw_pci_ops = &pci_sal_ops;
ah ok.
> by just assigning raw_pci_ops in their own code. I haven't looked at
> the SGI code yet, but this is how arch/i386/pci/direct.c (for example)
> works.
>
> > Maybe rename pci_root_ops to "acpi_pci_ops" would make that clearer.
>
> No. Don't rename it to anything ACPI specific. It isn't.
I understand raw_pci_ops is not ACPI specific.
But pci_root_ops is only used by pci_acpi_scan_root().
grundler@gsyprf3:/usr/src/linux-2.6.8.1$ fgrep pci_acpi_scan_root include/*/*
include/acpi/acpi_drivers.h:struct pci_bus *pci_acpi_scan_root(struct acpi_device *device, int domain, int bus);
and
./drivers/acpi/pci_root.c: root->bus = pci_acpi_scan_root(device, root->id.segment, root->id.bus);
The rename still seems appropriate to me.
thanks,
grant
On Tue, Oct 05, 2004 at 12:10:08PM -0700, Grant Grundler wrote:
> > > Maybe rename pci_root_ops to "acpi_pci_ops" would make that clearer.
> >
> > No. Don't rename it to anything ACPI specific. It isn't.
>
> I understand raw_pci_ops is not ACPI specific.
> But pci_root_ops is only used by pci_acpi_scan_root().
Yes, but if we had other ways of discovering PCI root bridges on ia64,
we would use it there too. It's exactly the same as the i386 code which
has 7 different ways to discover PCI root bridges.
--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain
>Yes, after looking at Grant's review/suggestion, we found that we can
>actually just use raw_pci_ops. This will work well for us. We have
>incoorporated this change. No changes in pci/pci.c needed.
Good. Let's try to make some forward progress here. I'd like
to see the patches broken into a sequence something like this:
1) Add new interfaces to header files to support any new API
needed by new files
2) Create all the new files (plain copies of old files where
a move is involved).
3) Functional changes to copied files.
4) Whitespace cleanup of copied files.
5) Point Makefiles to new files
6) Delete all the old/unused files.
7) Delete any API in headers that were only used by old files.
We'll need to coordinate with some other maintainrs for
drivers/pci/hotplug/Kconfig and drivers/scsi/qla1280.c,
but I'm ok with running all the other parts through the
ia64 tree.
This follows the usual guidelines of a sequence of steps where
the system is buildable+usable at each stage.
-Tony
Luck, Tony wrote:
>>Yes, after looking at Grant's review/suggestion, we found that we can
>>actually just use raw_pci_ops. This will work well for us. We have
>>incoorporated this change. No changes in pci/pci.c needed.
>
>
> Good. Let's try to make some forward progress here. I'd like
> to see the patches broken into a sequence something like this:
>
> 1) Add new interfaces to header files to support any new API
> needed by new files
> 2) Create all the new files (plain copies of old files where
> a move is involved).
> 3) Functional changes to copied files.
> 4) Whitespace cleanup of copied files.
> 5) Point Makefiles to new files
> 6) Delete all the old/unused files.
> 7) Delete any API in headers that were only used by old files.
>
> We'll need to coordinate with some other maintainrs for
> drivers/pci/hotplug/Kconfig and drivers/scsi/qla1280.c,
> but I'm ok with running all the other parts through the
> ia64 tree.
>
> This follows the usual guidelines of a sequence of steps where
> the system is buildable+usable at each stage.
>
Tony,
It had been suggested that we submit this as new code - since it can't be transitioned to. And I
thought that was what we had decided on - a 'kill' patch and an 'add' patch. I can remove any
Lindent'ing of older files if you don't want that. I will take out the Kconfig mod. I believe
Christoph is the maintainer of the qla driver (he was one of the reviewers).
-- Pat
>It had been suggested that we submit this as new code - since
>it can't be transitioned to. And I thought that was what we
>had decided on - a 'kill' patch and an 'add' patch.
Sorry ... I must have missed that.
>I can remove any Lindent'ing of older files if you don't want that.
Yes please.
>I will take out the Kconfig mod.
Good.
>I believe Christoph is the maintainer of the qla driver (he was one of
>the reviewers).
His fingerprints are all over the revision history. It looks like the
only real change you want here is deleting the ugly hack for SN2:
< #if defined(CONFIG_IA64_GENERIC) || defined(CONFIG_IA64_SGI_SN2)
< #include <asm/sn/pci/pciio.h>
< /* Ugly hack needed for the virtual channel fix on SN2 */
< extern int snia_pcibr_rrb_alloc(struct pci_dev *pci_dev,
< int *count_vchan0, int *count_vchan1);
< #endif
If Christoph signs off on that, then I can feed a separate patch
that does that at the same time as the kill/add.
-Tony
Christoph Hellwig wrote:
> This looks pretty nice already, but a few small but important issues
> need sorting out.
>
> - The interface between pci_dma.c and the lowlevel code is still wrong -
> and because of the additional 32bit direct translation in this code drop
> it got even worse because you might be calling into a function just to
> error out again.
> Please implement my suggestions from month ago, it's not a lot of work.
> - various sall baclls take bus_number and devfs but no pci domain, while
> the normal SAL calls do, I think you should make the kernel code aware
> of pci domains so the prom can introduce them seamlessly
> - is doing SAL calls from irq context really safe? Also why do you need
> different SAL calls for shub vs ice error? The prom should be easily
> able to find out what hub a given nasid corresponds to.
> - the patch reformats various unrelated or only slightly related files.
> Please don't do that - in general the new style is better than the old
> one, but it doesn't belong in this patchA
> - there's a SNDRV_SHUB_GET_IOCTL_VERSION ioctl define added but never
> used. In fact it looks like all SNDRV_SHUB_ values are unused now?
>
Christoph,
Thanks for the review. We're working on spinning up a new version with these changes.
Also the issue of where to put sn_pci_set_vchan().... I had originally put it in
include/asm-ia64/sn/io.h, but then this file doesn't get picked up for generic and others...
So I'm looking for some guidance... almost seems like putting it in qla1280.c is the most obvious -
since it is only used there and then there isn't include file fooling around to do.
-- Pat
Luck, Tony wrote:
>>It had been suggested that we submit this as new code - since
>>it can't be transitioned to. And I thought that was what we
>>had decided on - a 'kill' patch and an 'add' patch.
>
>
> Sorry ... I must have missed that.
>
>
>>I can remove any Lindent'ing of older files if you don't want that.
>
>
> Yes please.
>
>
>>I will take out the Kconfig mod.
>
>
> Good.
>
>
>>I believe Christoph is the maintainer of the qla driver (he was one of
>>the reviewers).
>
>
> His fingerprints are all over the revision history. It looks like the
> only real change you want here is deleting the ugly hack for SN2:
>
> < #if defined(CONFIG_IA64_GENERIC) || defined(CONFIG_IA64_SGI_SN2)
> < #include <asm/sn/pci/pciio.h>
> < /* Ugly hack needed for the virtual channel fix on SN2 */
> < extern int snia_pcibr_rrb_alloc(struct pci_dev *pci_dev,
> < int *count_vchan0, int *count_vchan1);
> < #endif
>
> If Christoph signs off on that, then I can feed a separate patch
> that does that at the same time as the kill/add.
>
> -Tony
>
Tony,
I've updated our ftp site with a new patch.
o Took out the Hotplug Kconfig mod (Tony's request)
o removed Lindent changes for non-sn files (Tony's request)
o SN_SAL_IOIF_RRB_ALLOC is gone (Christoph's request)
o added domain arg to the SAL calls that had bus/device (Christoph's request)
o improved pci_dma.c (Christoph's request)
o removed unused SNDRV_SHUB_??? defs (Christoph's request)
o added our own pci_ops (Grant/Matthew's request)
Patches are here:
ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
ftp://oss.sgi.com/projects/sn2/sn2-update/002-add-files
I also put a separate patch for the qla code:
ftp://oss.sgi.com/projects/sn2/sn2-update/003-qla-mod
On Wed, Oct 06, 2004 at 10:32:23AM -0500, Patrick Gefre wrote:
> o added our own pci_ops (Grant/Matthew's request)
Sorry - my bad.
I confused the issue by claiming one should replace pci_root_ops.
It was one possibility but it's not an easy path to take.
Mathew explained replacing the raw_pci_ops pointer is the Right Thing
and I suspect it's easier to properly implement.
Some comments on the implementation:
o sn_pci_fixup_bus() is a confusing name. "pcibios_fixup_bus" is normally
called by generic PCI code after each bus is walked.
This code obviously doesn't support that.
Maybe, sn_init_pci_controller() or something like that would be clearer.
o This bit of code belongs in the pcibios_fixup_bus() call path:
+ /*
+ * Generic Linux PCI Layer has created the pci_bus and pci_dev
+ * structures - time for us to add our SN PLatform specific
+ * information.
+ */
+
+ while ((pci_dev =
+ pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
+ sn_pci_fixup_slot(pci_dev);
+ }
I realize that's not easy to add/maintain in the arch/ia64 port though
since pcibios_fixup_bus() is common code for multiple platforms.
o sn_pci_fixup_bus() should be called for each PCI root bus controller
the firmware advertises. The loop in sn_pci_init() is hard coded
to loop from 0 to 256 busses.
Is ACPI the only way PCI host controllers are advertised?
SN2 doesn't use a different method today?
It means we are telling PCI subsystem to walk root busses that don't
exist in all configurations. I hope there are no nasty side effects
from that.
o the BUG() in:
+ controller = sn_alloc_pci_sysdata();
+ if (!controller) {
+ BUG();
+ }
is redundant with the BUG in sn_alloc_pci_sysdata().
sorry for the initial bad advice and I hope this helps,
grant
Grant Grundler wrote:
> On Wed, Oct 06, 2004 at 10:32:23AM -0500, Patrick Gefre wrote:
> > o added our own pci_ops (Grant/Matthew's request)
>
> Sorry - my bad.
> I confused the issue by claiming one should replace pci_root_ops.
> It was one possibility but it's not an easy path to take.
Hi Grant,
>
>
> Mathew explained replacing the raw_pci_ops pointer is the Right Thing
> and I suspect it's easier to properly implement.
I believe we did just that. We did not touch pci_root_ops.
>
>
> Some comments on the implementation:
> o sn_pci_fixup_bus() is a confusing name. "pcibios_fixup_bus" is normally
> called by generic PCI code after each bus is walked.
> This code obviously doesn't support that.
> Maybe, sn_init_pci_controller() or something like that would be clearer.
That is a good idea and can be done.
>
>
> o This bit of code belongs in the pcibios_fixup_bus() call path:
> + /*
> + * Generic Linux PCI Layer has created the pci_bus and pci_dev
> + * structures - time for us to add our SN PLatform specific
> + * information.
> + */
> +
> + while ((pci_dev =
> + pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
> + sn_pci_fixup_slot(pci_dev);
> + }
>
> I realize that's not easy to add/maintain in the arch/ia64 port though
> since pcibios_fixup_bus() is common code for multiple platforms.
Yes, would anybody allow us to make a platform specific callout from within generic
pcibios_fixup_bus()???
>
>
> o sn_pci_fixup_bus() should be called for each PCI root bus controller
> the firmware advertises. The loop in sn_pci_init() is hard coded
> to loop from 0 to 256 busses.
> Is ACPI the only way PCI host controllers are advertised?
That would be my assumption. And ACPI is our next effort.
>
> SN2 doesn't use a different method today?
Correct.
>
>
> It means we are telling PCI subsystem to walk root busses that don't
> exist in all configurations. I hope there are no nasty side effects
> from that.
Not at all. If you look at the loop, sn_pci_fixup_bus(0 gets called for 0 -
PCI_BUSES_TO_SCAN but if the bus does not exist, we do not call pci_scan_bus(),
therefore the PCI subsystem is not called to walk buses that do not exist on SN.
>
>
> o the BUG() in:
>
> + controller = sn_alloc_pci_sysdata();
> + if (!controller) {
> + BUG();
> + }
> is redundant with the BUG in sn_alloc_pci_sysdata().
Thanks. We can fix this.
One favour. Would you agree to letting this patch be included by Tony and we will come
up with another patch to fix the 2 obvious items listed above? It will be great to
avoid spinning this big patch.
Thanks.
colin
>
>
> sorry for the initial bad advice and I hope this helps,
> grant
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Grant Grundler wrote:
> Colin,
> thanks for ACKing the feedback.
> I think there is still some confusion...
>
> On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote:
> ...
>
>>>Mathew explained replacing the raw_pci_ops pointer is the Right Thing
>>>and I suspect it's easier to properly implement.
>>
>>I believe we did just that. We did not touch pci_root_ops.
>
>
> Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops
> (which is what I originally suggested).
>
> Mathew's point was only raw_pci_ops needs to point at a different
> set of struct pci_raw_ops (see include/linux/pci.h).
>
>
>>> I realize that's not easy to add/maintain in the arch/ia64 port though
>>> since pcibios_fixup_bus() is common code for multiple platforms.
>>
>>Yes, would anybody allow us to make a platform specific callout
>>from within generic pcibios_fixup_bus()???
>
>
> If it can be avoided, preferably not. But that's up to Jesse/Tony I think.
>
> ...
>
>>> It means we are telling PCI subsystem to walk root busses that don't
>>> exist in all configurations. I hope there are no nasty side effects
>>> from that.
>>
>>Not at all. If you look at the loop, sn_pci_fixup_bus(0 gets called for 0 -
>>PCI_BUSES_TO_SCAN but if the bus does not exist,
>
>
> Can you quote the bit of the patch which implements "if the bus does not
> exist" check?
> I can't find it.
>
>
>>One favour. Would you agree to letting this patch be included by Tony
>>and we will come up with another patch to fix the 2 obvious items listed
>>above? It will be great to avoid spinning this big patch.
>
>
> I think that's up to Jesse/Tony.
> I don't "own" any of the code in question.
> Just trying to undo the confusion I caused.
>
> grant
I don't plan on respinning the large patches (unless of course they get out of date). It would be
great to get the kill, add and qla patch in so we can move forward and address some these other
smaller issues - rather than holding up the larger mods for them.
-- Pat
Grant Grundler wrote:
> Colin,
> thanks for ACKing the feedback.
> I think there is still some confusion...
>
> On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote:
> ...
> > > Mathew explained replacing the raw_pci_ops pointer is the Right Thing
> > > and I suspect it's easier to properly implement.
> >
> > I believe we did just that. We did not touch pci_root_ops.
>
> Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops
> (which is what I originally suggested).
>
> Mathew's point was only raw_pci_ops needs to point at a different
> set of struct pci_raw_ops (see include/linux/pci.h).
Hi Grant,
Well, I am confused then.
Originally, we needed to use pci_root_ops in io_init.c to pass it to
pci_scan_bus(). But pci_root_ops is defined as a static in pci/pci.c. We took
out the static so that we can use this in io_init.c. However, it sounded like
you guys do not want to externalize pci_root_ops. Okay, we created
sn_pci_root_ops.
We do not want pci_raw_ops to point at anything different. It is exactly what we
needed now that we have implemented in our Prom all the pci config read/write SAL
calls.
>
>
> > > I realize that's not easy to add/maintain in the arch/ia64 port though
> > > since pcibios_fixup_bus() is common code for multiple platforms.
> >
> > Yes, would anybody allow us to make a platform specific callout
> > from within generic pcibios_fixup_bus()???
>
> If it can be avoided, preferably not. But that's up to Jesse/Tony I think.
>
> ...
> > > It means we are telling PCI subsystem to walk root busses that don't
> > > exist in all configurations. I hope there are no nasty side effects
> > > from that.
> >
> > Not at all. If you look at the loop, sn_pci_fixup_bus(0 gets called for 0 -
> > PCI_BUSES_TO_SCAN but if the bus does not exist,
>
> Can you quote the bit of the patch which implements "if the bus does not
> exist" check?
> I can't find it.
In the routine sn_pci_fixup_bus()
+static void sn_pci_fixup_bus(int segment, int busnum)
+{
+ int status = 0;
+ int nasid, cnode;
+ struct pci_bus *bus;
+ struct pci_controller *controller;
+ struct pcibus_bussoft *prom_bussoft_ptr;
+ struct hubdev_info *hubdev_info;
+ void *provider_soft;
+
+ status =
+ sal_get_pcibus_info((u64) segment, (u64) busnum,
+ (u64) ia64_tpa(&prom_bussoft_ptr));
+ if (status > 0) {
+ return; /* bus # does not exist */
+ }
+
+ prom_bussoft_ptr = __va(prom_bussoft_ptr);
+ controller = sn_alloc_pci_sysdata();
+ if (!controller) {
+ BUG();
+ }
+
+ bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller);
+ if (bus == NULL) {
+ return; /* error, or bus already scanned */
+ }
We bail if sal_get_pcibus_info() is not successful. Am I missing something?
>
>
> > One favour. Would you agree to letting this patch be included by Tony
> > and we will come up with another patch to fix the 2 obvious items listed
> > above? It will be great to avoid spinning this big patch.
>
> I think that's up to Jesse/Tony.
> I don't "own" any of the code in question.
> Just trying to undo the confusion I caused.
Thanks.
colin
>
>
> grant
Jesse Barnes wrote:
Hi Jesse/Grant,
May be my response to Grant got lost .. anyway, here it is again.
> On Wednesday, October 6, 2004 12:54 pm, Grant Grundler wrote:
> > Colin,
> > thanks for ACKing the feedback.
> > I think there is still some confusion...
> >
> > On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote:
> > ...
> >
> > > > Mathew explained replacing the raw_pci_ops pointer is the Right Thing
> > > > and I suspect it's easier to properly implement.
> > >
> > > I believe we did just that. We did not touch pci_root_ops.
> >
> > Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops
> > (which is what I originally suggested).
> >
> > Mathew's point was only raw_pci_ops needs to point at a different
> > set of struct pci_raw_ops (see include/linux/pci.h).
>
> Though now what's there seems awfully redundant, wouldn't you say? Just
> allowing direct access to pci_root_ops is a much simpler approach and gets
> rid of a bunch of extra, unneeded code (i.e. closer to Pat's original
> version).
The original mod, we took out the static from pci_root_ops() so that we can use
it in io_init.c. We thought that would be the cleanest.
We do not want to change pci_raw_ops(). It is doing exactly what we need, now
that sn platform has the support for SAL pci reads and writes support.
>
>
> > > Yes, would anybody allow us to make a platform specific callout
> > > from within generic pcibios_fixup_bus()???
> >
> > If it can be avoided, preferably not. But that's up to Jesse/Tony I think.
>
> If it was made a machine vector that's a no-op on everything but sn2, I think
> it would be fine. Doing it for the general sn_pci_init routine would let us
> get rid of the check for ia64_platform_is("sn2") in one of the routines, I
> think (which is nice if only for the consistency).
>
> > Can you quote the bit of the patch which implements "if the bus does not
> > exist" check?
> > I can't find it.
>
> In the current code it's:
>
> for (i = 0; i < PCI_BUSES_TO_SCAN; i++)
> if (pci_bus_to_vertex(i))
> pci_scan_bus(i, &sn_pci_ops, controller);
>
> which causes the next loop to only fixup existing busses. But I don't see it
> in the new code.
Probably not clear to all:
+/*
+ * sn_pci_fixup_bus() - This routine sets up a bus's resources
+ * consistent with the Linux PCI abstraction layer.
+ */
+static void sn_pci_fixup_bus(int segment, int busnum)
+{
+ int status = 0;
+ int nasid, cnode;
+ struct pci_bus *bus;
+ struct pci_controller *controller;
+ struct pcibus_bussoft *prom_bussoft_ptr;
+ struct hubdev_info *hubdev_info;
+ void *provider_soft;
+
+ status =
+ sal_get_pcibus_info((u64) segment, (u64) busnum,
+ (u64) ia64_tpa(&prom_bussoft_ptr));
+ if (status > 0) {
+ return; /* bus # does not exist */
+ }
+
+ prom_bussoft_ptr = __va(prom_bussoft_ptr);
+ controller = sn_alloc_pci_sysdata();
+ if (!controller) {
+ BUG();
+ }
+
+ bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller);
+ if (bus == NULL) {
+ return; /* error, or bus already scanned */
+ }
The sal_get_pcibus_info() will fail if we do not find that bus number. If it
fails, we do not call pci_scan_bus()
Thanks.
colin
>
>
> > > One favour. Would you agree to letting this patch be included by Tony
> > > and we will come up with another patch to fix the 2 obvious items listed
> > > above? It will be great to avoid spinning this big patch.
>
> The patch is ok with me, I think it's a big improvement over what's there in
> terms of readability.
>
> I just checked out sn_set_affinity_irq() and it's a bit hard to see what's
> going on. Why does a new interrupt have to be allocated? Also, it looks
> like the kfree() is one line too high, if sn_intr_alloc fails, we'll leak
> new_sn_irq_info.
>
> Jesse
On Wed, Oct 06, 2004 at 01:27:28PM -0700, Jesse Barnes wrote:
> Though now what's there seems awfully redundant, wouldn't you say? Just
> allowing direct access to pci_root_ops is a much simpler approach and gets
> rid of a bunch of extra, unneeded code (i.e. closer to Pat's original
> version).
now that I understand what's going on, I think it's better to make
pci_root_ops non-static. Of course, it'd be better if SN2 used acpi to
discover its root busses, but I guess that'll take longer to implement.
--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain
On Wed, Oct 06, 2004 at 01:27:28PM -0700, Jesse Barnes wrote:
> Though now what's there seems awfully redundant, wouldn't you say?
Yes - but we are using code which *requires* the arch define raw_pci_ops.
See drivers/acpi/osl.c:acpi_os_initialize()
#ifdef CONFIG_ACPI_PCI
if (!raw_pci_ops) {
printk(KERN_ERR PREFIX "Access to PCI configuration space unavailable\n");
return AE_NULL_ENTRY;
}
#endif
> Just
> allowing direct access to pci_root_ops is a much simpler approach and gets
> rid of a bunch of extra, unneeded code (i.e. closer to Pat's original
> version).
Agreed. I'm not real clear on why drivers/acpi didn't do that.
But apperently ACPI supports many methods to PCI or PCI-Like (can you
guess I'm not clear on this?) config space. raw_pci_ops supports
multiple methods in i386. ia64 only happens to use one so far.
It seems right for SN2 to use this mechanism if it needs a different
method.
Willy tried to explain this to me yesterday and I thought I understood
most of it...apperently that was a transient moment of clarity. :^/
...
> > Can you quote the bit of the patch which implements "if the bus does not
> > exist" check?
> > I can't find it.
>
> In the current code it's:
>
> for (i = 0; i < PCI_BUSES_TO_SCAN; i++)
> if (pci_bus_to_vertex(i))
> pci_scan_bus(i, &sn_pci_ops, controller);
>
> which causes the next loop to only fixup existing busses. But I don't see it
> in the new code.
Ok. I'm glad it's not just me 'cuz I'm having a bad day.
thanks,
grant
Matthew Wilcox wrote:
> On Wed, Oct 06, 2004 at 01:48:32PM -0700, Grant Grundler wrote:
> > Agreed. I'm not real clear on why drivers/acpi didn't do that.
> > But apperently ACPI supports many methods to PCI or PCI-Like (can you
> > guess I'm not clear on this?) config space. raw_pci_ops supports
> > multiple methods in i386. ia64 only happens to use one so far.
> > It seems right for SN2 to use this mechanism if it needs a different
> > method.
> >
> > Willy tried to explain this to me yesterday and I thought I understood
> > most of it...apperently that was a transient moment of clarity. :^/
>
> Let's try it again, by email this time.
>
> Fundamentally, there is a huge impedence mismatch between how the ACPI
> interpreter wants to access PCI configuration space, and how Linux wants
> to access PCI configuration space. Linux always has at least a pci_bus
> around, if not a pci_dev. So we can use dev->bus->ops to abstract the
> architecture-specific implementation of "how do I get to configuration
> space for this bus?"
>
> ACPI doesn't have a pci_bus. It just passes around a struct of { domain,
> bus, dev, function } and expects the OS-specific code to determine what
> to do with it. The original hacky code constructed a fake pci_dev on the
> stack and called the regular methods. This broke ia64 because we needed
> something else to be valid (I forget what), so as part of the grand "get
> ia64 fully merged upstream" effort, I redesigned the OS-specific code.
>
> Fortunately, neither i386 nor ia64 actually need the feature Linux has
> to have a per-bus pci_ops -- it's always the same. I think powerpc is
> the only architecture that needs it. So I introduced a pci_raw_ops that
> both ACPI and a generic pci_root_ops could call.
>
> The part I didn't seem to be able to get across to you yesterday was
> that pci_root_ops is not just used for the PCI root bridge, it's used
> for accessing every PCI device underneath that root bridge.
Hi Guys,
Therefore, would it be perfectly fine if we remove the static from pci_root_ops
so that we can use it outside of pci/pci.c?? I can include this in a follow-on
patch.
Thanks.
colin
>
>
> --
> "Next the statesmen will invent cheap lies, putting the blame upon
> the nation that is attacked, and every man will be glad of those
> conscience-soothing falsities, and will diligently study them, and refuse
> to examine any refutations of them; and thus he will by and by convince
> himself that the war is just, and will thank God for the better sleep
> he enjoys after this process of grotesque self-deception." -- Mark Twain
On Wed, Oct 06, 2004 at 01:48:32PM -0700, Grant Grundler wrote:
> Agreed. I'm not real clear on why drivers/acpi didn't do that.
> But apperently ACPI supports many methods to PCI or PCI-Like (can you
> guess I'm not clear on this?) config space. raw_pci_ops supports
> multiple methods in i386. ia64 only happens to use one so far.
> It seems right for SN2 to use this mechanism if it needs a different
> method.
>
> Willy tried to explain this to me yesterday and I thought I understood
> most of it...apperently that was a transient moment of clarity. :^/
Let's try it again, by email this time.
Fundamentally, there is a huge impedence mismatch between how the ACPI
interpreter wants to access PCI configuration space, and how Linux wants
to access PCI configuration space. Linux always has at least a pci_bus
around, if not a pci_dev. So we can use dev->bus->ops to abstract the
architecture-specific implementation of "how do I get to configuration
space for this bus?"
ACPI doesn't have a pci_bus. It just passes around a struct of { domain,
bus, dev, function } and expects the OS-specific code to determine what
to do with it. The original hacky code constructed a fake pci_dev on the
stack and called the regular methods. This broke ia64 because we needed
something else to be valid (I forget what), so as part of the grand "get
ia64 fully merged upstream" effort, I redesigned the OS-specific code.
Fortunately, neither i386 nor ia64 actually need the feature Linux has
to have a per-bus pci_ops -- it's always the same. I think powerpc is
the only architecture that needs it. So I introduced a pci_raw_ops that
both ACPI and a generic pci_root_ops could call.
The part I didn't seem to be able to get across to you yesterday was
that pci_root_ops is not just used for the PCI root bridge, it's used
for accessing every PCI device underneath that root bridge.
--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain
On Wednesday, October 6, 2004 1:10 pm, Patrick Gefre wrote:
> I don't plan on respinning the large patches (unless of course they get out
> of date). It would be great to get the kill, add and qla patch in so we can
> move forward and address some these other smaller issues - rather than
> holding up the larger mods for them.
I agree, but could you please just 'vi' the 002-add-files patch and remove
these?
drivers/char/mmtimer.c | 1
drivers/char/snsc.c | 25
drivers/ide/pci/sgiioc4.c | 23
drivers/serial/sn_console.c | 214
They should each be separate cleanup patches. What I've done in the past is
make copies (in this case 5) of the big patch. Then I edit all of them to
include only the hunks I want there. Hopefully that'll minimize the pain of
respinning the big patch (i.e. no respin). Also, Tony doesn't want to deal
with the above files, patches for them should be sent to Andrew as separate
mails with lkml in the cc list.
Other than that, I'm all for getting these into the tree. Thanks for all the
work you've put into this!
Thanks,
Jesse
On Wednesday, October 6, 2004 12:54 pm, Grant Grundler wrote:
> Colin,
> thanks for ACKing the feedback.
> I think there is still some confusion...
>
> On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote:
> ...
>
> > > Mathew explained replacing the raw_pci_ops pointer is the Right Thing
> > > and I suspect it's easier to properly implement.
> >
> > I believe we did just that. We did not touch pci_root_ops.
>
> Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops
> (which is what I originally suggested).
>
> Mathew's point was only raw_pci_ops needs to point at a different
> set of struct pci_raw_ops (see include/linux/pci.h).
Though now what's there seems awfully redundant, wouldn't you say? Just
allowing direct access to pci_root_ops is a much simpler approach and gets
rid of a bunch of extra, unneeded code (i.e. closer to Pat's original
version).
> > Yes, would anybody allow us to make a platform specific callout
> > from within generic pcibios_fixup_bus()???
>
> If it can be avoided, preferably not. But that's up to Jesse/Tony I think.
If it was made a machine vector that's a no-op on everything but sn2, I think
it would be fine. Doing it for the general sn_pci_init routine would let us
get rid of the check for ia64_platform_is("sn2") in one of the routines, I
think (which is nice if only for the consistency).
> Can you quote the bit of the patch which implements "if the bus does not
> exist" check?
> I can't find it.
In the current code it's:
for (i = 0; i < PCI_BUSES_TO_SCAN; i++)
if (pci_bus_to_vertex(i))
pci_scan_bus(i, &sn_pci_ops, controller);
which causes the next loop to only fixup existing busses. But I don't see it
in the new code.
> > One favour. Would you agree to letting this patch be included by Tony
> > and we will come up with another patch to fix the 2 obvious items listed
> > above? It will be great to avoid spinning this big patch.
The patch is ok with me, I think it's a big improvement over what's there in
terms of readability.
I just checked out sn_set_affinity_irq() and it's a bit hard to see what's
going on. Why does a new interrupt have to be allocated? Also, it looks
like the kfree() is one line too high, if sn_intr_alloc fails, we'll leak
new_sn_irq_info.
Jesse
Colin,
thanks for ACKing the feedback.
I think there is still some confusion...
On Wed, Oct 06, 2004 at 02:09:54PM -0500, Colin Ngam wrote:
...
> > Mathew explained replacing the raw_pci_ops pointer is the Right Thing
> > and I suspect it's easier to properly implement.
>
> I believe we did just that. We did not touch pci_root_ops.
Correct. The patch ignores/overides pci_root_ops with sn_pci_root_ops
(which is what I originally suggested).
Mathew's point was only raw_pci_ops needs to point at a different
set of struct pci_raw_ops (see include/linux/pci.h).
> > I realize that's not easy to add/maintain in the arch/ia64 port though
> > since pcibios_fixup_bus() is common code for multiple platforms.
>
> Yes, would anybody allow us to make a platform specific callout
> from within generic pcibios_fixup_bus()???
If it can be avoided, preferably not. But that's up to Jesse/Tony I think.
...
> > It means we are telling PCI subsystem to walk root busses that don't
> > exist in all configurations. I hope there are no nasty side effects
> > from that.
>
> Not at all. If you look at the loop, sn_pci_fixup_bus(0 gets called for 0 -
> PCI_BUSES_TO_SCAN but if the bus does not exist,
Can you quote the bit of the patch which implements "if the bus does not
exist" check?
I can't find it.
> One favour. Would you agree to letting this patch be included by Tony
> and we will come up with another patch to fix the 2 obvious items listed
> above? It will be great to avoid spinning this big patch.
I think that's up to Jesse/Tony.
I don't "own" any of the code in question.
Just trying to undo the confusion I caused.
grant
Jesse Barnes wrote:
> On Wednesday, October 6, 2004 1:10 pm, Patrick Gefre wrote:
>
>>I don't plan on respinning the large patches (unless of course they get out
>>of date). It would be great to get the kill, add and qla patch in so we can
>>move forward and address some these other smaller issues - rather than
>>holding up the larger mods for them.
>
>
> I agree, but could you please just 'vi' the 002-add-files patch and remove
> these?
>
> drivers/char/mmtimer.c | 1
> drivers/char/snsc.c | 25
> drivers/ide/pci/sgiioc4.c | 23
> drivers/serial/sn_console.c | 214
>
> They should each be separate cleanup patches. What I've done in the past is
> make copies (in this case 5) of the big patch. Then I edit all of them to
> include only the hunks I want there. Hopefully that'll minimize the pain of
> respinning the big patch (i.e. no respin). Also, Tony doesn't want to deal
> with the above files, patches for them should be sent to Andrew as separate
> mails with lkml in the cc list.
>
These are not cleanup.
The mmtimer code and sn_console include a file that doesn't exist anymore in the directory included
- it's moved to somewhere else in the 002 patch.
snsc.c, sgiioc4.c have changes for things that won't exist after this patch is applied.
On Thursday, October 7, 2004 10:06 am, Luck, Tony wrote:
> >Yeah, sorry, I shouldn't have said cleanup, fixup is better.
> >Anyway, they
> >need to be separate since they'll be going into the tree via
> >Andrew not Tony.
>
> A couple of days back I said that I'm ok pushing these drivers.
> Although they don't have "arch/ia64" or "include/asm-ia64"
> prefixes, they are only used by ia64. I'm even ok with the
> qla1280.c change as the final version is only touching code
> inside #ifdef CONFIG_IA64_{GENERIC|SN2) ... but I would like
> to see a sign-off from the de-facto maintainer Christoph for
> this file.
Ok great, that'll help keep things in good shape.
> However ... there's a thread on LKML wailing about huge changes
> going into "-rc" releases. Since there still seems to be
> a lively discussion about the the right way to do the pci_root
> bits of this patch, I'm very inclined to save this till *after*
> Linus release 2.6.9-final. If there's a _mostly_ clean patch
> presented to me before 2.6.10-rc1 shows up, I'll push that and
> allow for some follow-on tidy-up patches to clean up.
Sounds good, thanks Tony.
Jesse
On Thursday, October 7, 2004 8:02 am, Patrick Gefre wrote:
> > They should each be separate cleanup patches. What I've done in the past
> > is make copies (in this case 5) of the big patch. Then I edit all of
> > them to include only the hunks I want there. Hopefully that'll minimize
> > the pain of respinning the big patch (i.e. no respin). Also, Tony
> > doesn't want to deal with the above files, patches for them should be
> > sent to Andrew as separate mails with lkml in the cc list.
>
> These are not cleanup.
>
> The mmtimer code and sn_console include a file that doesn't exist anymore
> in the directory included - it's moved to somewhere else in the 002 patch.
>
> snsc.c, sgiioc4.c have changes for things that won't exist after this patch
> is applied.
Yeah, sorry, I shouldn't have said cleanup, fixup is better. Anyway, they
need to be separate since they'll be going into the tree via Andrew not Tony.
Thanks,
Jesse
>Yeah, sorry, I shouldn't have said cleanup, fixup is better.
>Anyway, they
>need to be separate since they'll be going into the tree via
>Andrew not Tony.
A couple of days back I said that I'm ok pushing these drivers.
Although they don't have "arch/ia64" or "include/asm-ia64"
prefixes, they are only used by ia64. I'm even ok with the
qla1280.c change as the final version is only touching code
inside #ifdef CONFIG_IA64_{GENERIC|SN2) ... but I would like
to see a sign-off from the de-facto maintainer Christoph for
this file.
This is not a land-grab to expand my responsibilities, it just
seems to be the right thing to do to coordinate getting all
these interdependent pieces into the tree at the same time.
However ... there's a thread on LKML wailing about huge changes
going into "-rc" releases. Since there still seems to be
a lively discussion about the the right way to do the pci_root
bits of this patch, I'm very inclined to save this till *after*
Linus release 2.6.9-final. If there's a _mostly_ clean patch
presented to me before 2.6.10-rc1 shows up, I'll push that and
allow for some follow-on tidy-up patches to clean up.
-Tony
>>>>> "Luck," == Luck, Tony <[email protected]> writes:
>> Yeah, sorry, I shouldn't have said cleanup, fixup is better.
>> Anyway, they need to be separate since they'll be going into the
>> tree via Andrew not Tony.
Luck,> A couple of days back I said that I'm ok pushing these drivers.
Luck,> Although they don't have "arch/ia64" or "include/asm-ia64"
Luck,> prefixes, they are only used by ia64. I'm even ok with the
Luck,> qla1280.c change as the final version is only touching code
Luck,> inside #ifdef CONFIG_IA64_{GENERIC|SN2) ... but I would like to
Luck,> see a sign-off from the de-facto maintainer Christoph for this
Luck,> file.
Tony,
As the maintainer for qla1280, I'll be happy sign off on the SN2
related changes. In fact it's a cleanup compared to the older hack I
implemented.
Cheers,
Jes
On Friday, October 8, 2004 8:16 am, Colin Ngam wrote:
> Now, if we can remove the static from pci_root_ops, I can use it in
> io_init.c, that would be cleanest and that was what we started with.
> This is what the patch would look like ontop of the 002_add* patch:
Yep, this looks good to me Colin.
> +extern struct pci_ops pci_root_ops;
> +
> +extern struct pci_ops pci_root_ops;
> +
...but of course you don't need to extern it twice :)
> We need a resolution so that Tony can proceed. Silence is not going to
> help.
You've got my vote.
Jesse
Colin Ngam wrote:
Gentlemen,
I need you to say yes or no on this issue so that Tony can proceed with
his decision on the next step towards this patch. Tony believes that
this is still an outstanding issue. Basically, it does not matter which
way we go.
In the current patch, because pci_root_ops is static, we define
sn_pci_root_ops. sn_pci_root_ops ends up calling raw_pci_ops - which is
exactly what we need.
Now, if we can remove the static from pci_root_ops, I can use it in
io_init.c, that would be cleanest and that was what we started with.
This is what the patch would look like ontop of the 002_add* patch:
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/10/07 11:53:49-05:00 [email protected]
# pci.h:
# Add prototype for pci_root_ops.
# io_init.c:
# Use pci_root_ops.
# pci.c:
# Remove static so that pci_root_ops can be externed.
#
# include/asm-ia64/pci.h
# 2004/10/07 11:53:02-05:00 [email protected] +2 -0
# Add prototype for pci_root_ops.
#
# arch/ia64/sn/kernel/io_init.c
# 2004/10/07 11:52:49-05:00 [email protected] +4 -27
# Use pci_root_ops.
#
# arch/ia64/pci/pci.c
# 2004/10/07 11:52:30-05:00 [email protected] +1 -1
# Remove static so that pci_root_ops can be externed.
#
diff -Nru a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
--- a/arch/ia64/pci/pci.c 2004-10-07 11:54:17 -05:00
+++ b/arch/ia64/pci/pci.c 2004-10-07 11:54:17 -05:00
@@ -124,7 +124,7 @@
devfn, where, size, value);
}
-static struct pci_ops pci_root_ops = {
+struct pci_ops pci_root_ops = {
.read = pci_read,
.write = pci_write,
};
diff -Nru a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
--- a/arch/ia64/sn/kernel/io_init.c 2004-10-07 11:54:17 -05:00
+++ b/arch/ia64/sn/kernel/io_init.c 2004-10-07 11:54:17 -05:00
@@ -33,29 +33,6 @@
int sn_ioif_inited = 0; /* SN I/O infrastructure
initialized? */
-static int
-sn_pci_read(struct pci_bus *bus, unsigned int devfn, int where, int size,
- u32 * value)
-{
- return raw_pci_ops->read(pci_domain_nr(bus), bus->number,
- devfn, where, size, value);
-}
-
-static int
-sn_pci_write(struct pci_bus *bus, unsigned int devfn, int where, int size,
- u32 value)
-{
-
- return raw_pci_ops->write(pci_domain_nr(bus), bus->number,
- devfn, where, size, value);
-}
-
-struct pci_ops sn_pci_root_ops = {
- .read = sn_pci_read,
- .write = sn_pci_write,
-};
-
-
/*
* Retrieve the DMA Flush List given nasid. This list is needed
* to implement the WAR - Flush DMA data on PIO Reads.
@@ -281,10 +258,10 @@
}
/*
- * sn_pci_fixup_bus() - This routine sets up a bus's resources
+ * sn_pci_controller_fixup() - This routine sets up a bus's resources
* consistent with the Linux PCI abstraction layer.
*/
-static void sn_pci_fixup_bus(int segment, int busnum)
+static void sn_pci_controller_fixup(int segment, int busnum)
{
int status = 0;
int nasid, cnode;
@@ -307,7 +284,7 @@
BUG();
}
- bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller);
+ bus = pci_scan_bus(busnum, &pci_root_ops, controller);
if (bus == NULL) {
return; /* error, or bus already scanned */
}
@@ -379,7 +356,7 @@
#endif
for (i = 0; i < PCI_BUSES_TO_SCAN; i++) {
- sn_pci_fixup_bus(0, i);
+ sn_pci_controller_fixup(0, i);
}
/*
diff -Nru a/include/asm-ia64/pci.h b/include/asm-ia64/pci.h
--- a/include/asm-ia64/pci.h 2004-10-07 11:54:17 -05:00
+++ b/include/asm-ia64/pci.h 2004-10-07 11:54:17 -05:00
@@ -105,6 +105,8 @@
#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
#define pci_domain_nr(busdev) (PCI_CONTROLLER(busdev)->segment)
+extern struct pci_ops pci_root_ops;
+
static inline int pci_name_bus(char *name, struct pci_bus *bus)
{
if (pci_domain_nr(bus) == 0) {
Basically, we add an extern for pci_root_ops in asm-ia64/pci.h and
remove the static for pci_root_ops in ia64/pci/pci.c.
We need a resolution so that Tony can proceed. Silence is not going to
help.
Thank you so much for your help.
colin
>Matthew Wilcox wrote:
>
>
>
>>On Wed, Oct 06, 2004 at 01:48:32PM -0700, Grant Grundler wrote:
>>
>>
>>>Agreed. I'm not real clear on why drivers/acpi didn't do that.
>>>But apperently ACPI supports many methods to PCI or PCI-Like (can you
>>>guess I'm not clear on this?) config space. raw_pci_ops supports
>>>multiple methods in i386. ia64 only happens to use one so far.
>>>It seems right for SN2 to use this mechanism if it needs a different
>>>method.
>>>
>>>Willy tried to explain this to me yesterday and I thought I understood
>>>most of it...apperently that was a transient moment of clarity. :^/
>>>
>>>
>>Let's try it again, by email this time.
>>
>>Fundamentally, there is a huge impedence mismatch between how the ACPI
>>interpreter wants to access PCI configuration space, and how Linux wants
>>to access PCI configuration space. Linux always has at least a pci_bus
>>around, if not a pci_dev. So we can use dev->bus->ops to abstract the
>>architecture-specific implementation of "how do I get to configuration
>>space for this bus?"
>>
>>ACPI doesn't have a pci_bus. It just passes around a struct of { domain,
>>bus, dev, function } and expects the OS-specific code to determine what
>>to do with it. The original hacky code constructed a fake pci_dev on the
>>stack and called the regular methods. This broke ia64 because we needed
>>something else to be valid (I forget what), so as part of the grand "get
>>ia64 fully merged upstream" effort, I redesigned the OS-specific code.
>>
>>Fortunately, neither i386 nor ia64 actually need the feature Linux has
>>to have a per-bus pci_ops -- it's always the same. I think powerpc is
>>the only architecture that needs it. So I introduced a pci_raw_ops that
>>both ACPI and a generic pci_root_ops could call.
>>
>>The part I didn't seem to be able to get across to you yesterday was
>>that pci_root_ops is not just used for the PCI root bridge, it's used
>>for accessing every PCI device underneath that root bridge.
>>
>>
>
>Hi Guys,
>
>Therefore, would it be perfectly fine if we remove the static from pci_root_ops
>so that we can use it outside of pci/pci.c?? I can include this in a follow-on
>patch.
>
>Thanks.
>
>colin
>
>
>
>>--
>>"Next the statesmen will invent cheap lies, putting the blame upon
>>the nation that is attacked, and every man will be glad of those
>>conscience-soothing falsities, and will diligently study them, and refuse
>>to examine any refutations of them; and thus he will by and by convince
>>himself that the war is just, and will thank God for the better sleep
>>he enjoys after this process of grotesque self-deception." -- Mark Twain
>>
>>
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
>
>
Colin Ngam wrote:
Gentlemen,
I need you to say yes or no on this issue so that Tony can proceed with
his decision on the next step towards this patch. Tony believes that
this is still an outstanding issue. Basically, it does not matter which
way we go.
In the current patch, because pci_root_ops is static, we define
sn_pci_root_ops. sn_pci_root_ops ends up calling raw_pci_ops - which is
exactly what we need.
Now, if we can remove the static from pci_root_ops, I can use it in
io_init.c, that would be cleanest and that was what we started with.
This is what the patch would look like ontop of the 002_add* patch:
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/10/07 11:53:49-05:00 [email protected]
# pci.h:
# Add prototype for pci_root_ops.
# io_init.c:
# Use pci_root_ops.
# pci.c:
# Remove static so that pci_root_ops can be externed.
#
# include/asm-ia64/pci.h
# 2004/10/07 11:53:02-05:00 [email protected] +2 -0
# Add prototype for pci_root_ops.
#
# arch/ia64/sn/kernel/io_init.c
# 2004/10/07 11:52:49-05:00 [email protected] +4 -27
# Use pci_root_ops.
#
# arch/ia64/pci/pci.c
# 2004/10/07 11:52:30-05:00 [email protected] +1 -1
# Remove static so that pci_root_ops can be externed.
#
diff -Nru a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
--- a/arch/ia64/pci/pci.c 2004-10-07 11:54:17 -05:00
+++ b/arch/ia64/pci/pci.c 2004-10-07 11:54:17 -05:00
@@ -124,7 +124,7 @@
devfn, where, size, value);
}
-static struct pci_ops pci_root_ops = {
+struct pci_ops pci_root_ops = {
.read = pci_read,
.write = pci_write,
};
diff -Nru a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
--- a/arch/ia64/sn/kernel/io_init.c 2004-10-07 11:54:17 -05:00
+++ b/arch/ia64/sn/kernel/io_init.c 2004-10-07 11:54:17 -05:00
@@ -33,29 +33,6 @@
int sn_ioif_inited = 0; /* SN I/O infrastructure
initialized? */
-static int
-sn_pci_read(struct pci_bus *bus, unsigned int devfn, int where, int size,
- u32 * value)
-{
- return raw_pci_ops->read(pci_domain_nr(bus), bus->number,
- devfn, where, size, value);
-}
-
-static int
-sn_pci_write(struct pci_bus *bus, unsigned int devfn, int where, int size,
- u32 value)
-{
-
- return raw_pci_ops->write(pci_domain_nr(bus), bus->number,
- devfn, where, size, value);
-}
-
-struct pci_ops sn_pci_root_ops = {
- .read = sn_pci_read,
- .write = sn_pci_write,
-};
-
-
/*
* Retrieve the DMA Flush List given nasid. This list is needed
* to implement the WAR - Flush DMA data on PIO Reads.
@@ -281,10 +258,10 @@
}
/*
- * sn_pci_fixup_bus() - This routine sets up a bus's resources
+ * sn_pci_controller_fixup() - This routine sets up a bus's resources
* consistent with the Linux PCI abstraction layer.
*/
-static void sn_pci_fixup_bus(int segment, int busnum)
+static void sn_pci_controller_fixup(int segment, int busnum)
{
int status = 0;
int nasid, cnode;
@@ -307,7 +284,7 @@
BUG();
}
- bus = pci_scan_bus(busnum, &sn_pci_root_ops, controller);
+ bus = pci_scan_bus(busnum, &pci_root_ops, controller);
if (bus == NULL) {
return; /* error, or bus already scanned */
}
@@ -379,7 +356,7 @@
#endif
for (i = 0; i < PCI_BUSES_TO_SCAN; i++) {
- sn_pci_fixup_bus(0, i);
+ sn_pci_controller_fixup(0, i);
}
/*
diff -Nru a/include/asm-ia64/pci.h b/include/asm-ia64/pci.h
--- a/include/asm-ia64/pci.h 2004-10-07 11:54:17 -05:00
+++ b/include/asm-ia64/pci.h 2004-10-07 11:54:17 -05:00
@@ -105,6 +105,8 @@
#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
#define pci_domain_nr(busdev) (PCI_CONTROLLER(busdev)->segment)
+extern struct pci_ops pci_root_ops;
+
+extern struct pci_ops pci_root_ops;
+
static inline int pci_name_bus(char *name, struct pci_bus *bus)
{
if (pci_domain_nr(bus) == 0) {
Basically, we add an extern for pci_root_ops in asm-ia64/pci.h and
remove the static for pci_root_ops in ia64/pci/pci.c.
We need a resolution so that Tony can proceed. Silence is not going to
help.
Thank you so much for your help.
colin
>Matthew Wilcox wrote:
>
>
>
>>On Wed, Oct 06, 2004 at 01:48:32PM -0700, Grant Grundler wrote:
>>
>>
>>>Agreed. I'm not real clear on why drivers/acpi didn't do that.
>>>But apperently ACPI supports many methods to PCI or PCI-Like (can you
>>>guess I'm not clear on this?) config space. raw_pci_ops supports
>>>multiple methods in i386. ia64 only happens to use one so far.
>>>It seems right for SN2 to use this mechanism if it needs a different
>>>method.
>>>
>>>Willy tried to explain this to me yesterday and I thought I understood
>>>most of it...apperently that was a transient moment of clarity. :^/
>>>
>>>
>>Let's try it again, by email this time.
>>
>>Fundamentally, there is a huge impedence mismatch between how the ACPI
>>interpreter wants to access PCI configuration space, and how Linux wants
>>to access PCI configuration space. Linux always has at least a pci_bus
>>around, if not a pci_dev. So we can use dev->bus->ops to abstract the
>>architecture-specific implementation of "how do I get to configuration
>>space for this bus?"
>>
>>ACPI doesn't have a pci_bus. It just passes around a struct of { domain,
>>bus, dev, function } and expects the OS-specific code to determine what
>>to do with it. The original hacky code constructed a fake pci_dev on the
>>stack and called the regular methods. This broke ia64 because we needed
>>something else to be valid (I forget what), so as part of the grand "get
>>ia64 fully merged upstream" effort, I redesigned the OS-specific code.
>>
>>Fortunately, neither i386 nor ia64 actually need the feature Linux has
>>to have a per-bus pci_ops -- it's always the same. I think powerpc is
>>the only architecture that needs it. So I introduced a pci_raw_ops that
>>both ACPI and a generic pci_root_ops could call.
>>
>>The part I didn't seem to be able to get across to you yesterday was
>>that pci_root_ops is not just used for the PCI root bridge, it's used
>>for accessing every PCI device underneath that root bridge.
>>
>>
>
>Hi Guys,
>
>Therefore, would it be perfectly fine if we remove the static from pci_root_ops
>so that we can use it outside of pci/pci.c?? I can include this in a follow-on
>patch.
>
>Thanks.
>
>colin
>
>
>
>>--
>>"Next the statesmen will invent cheap lies, putting the blame upon
>>the nation that is attacked, and every man will be glad of those
>>conscience-soothing falsities, and will diligently study them, and refuse
>>to examine any refutations of them; and thus he will by and by convince
>>himself that the war is just, and will thank God for the better sleep
>>he enjoys after this process of grotesque self-deception." -- Mark Twain
>>
>>
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
>
>
On Fri, Oct 08, 2004 at 10:16:13AM -0500, Colin Ngam wrote:
> Now, if we can remove the static from pci_root_ops, I can use it in
> io_init.c, that would be cleanest and that was what we started with.
willy already agreed:
http://marc.theaimsgroup.com/?l=linux-ia64&m=109709521721980&w=2
I'm ok with it too.
hth,
grant
Tony,
We came to a resolution on the pci_root-ops issue, Jesse is OK with the code, Jes and Christoph are
fine with the qla mod. I've added a couple of fixes from us as well as removing a redundant check
pointed out in the review - see the full list below. So the code is ready to go.
Can you take this now Tony ?
Thanks,
-- Pat
ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
ftp://oss.sgi.com/projects/sn2/sn2-update/002-add-files
ftp://oss.sgi.com/projects/sn2/sn2-update/003-qla-mod
ftp://oss.sgi.com/projects/sn2/sn2-update/004-sn_hwperf - fix from us
ftp://oss.sgi.com/projects/sn2/sn2-update/005-redundant-check-killed - removed rundundant check
ftp://oss.sgi.com/projects/sn2/sn2-update/006-sn_set_affinity_irq - fix from us
ftp://oss.sgi.com/projects/sn2/sn2-update/007-root-ops - make pci_root_ops non-static
Colin Ngam wrote:
> Grant Grundler wrote:
>
> Hi Tony,
>
> Jesse is alright with this issue too. Unfortunately, I believe his
> email may not have gotten out of SGI because we were having email
> problems on Friday.
>
> Thanks gents.
>
> colin
>
>> On Fri, Oct 08, 2004 at 10:16:13AM -0500, Colin Ngam wrote:
>> > Now, if we can remove the static from pci_root_ops, I can use it in
>> > io_init.c, that would be cleanest and that was what we started with.
>>
>> willy already agreed:
>> http://marc.theaimsgroup.com/?l=linux-ia64&m=109709521721980&w=2
>> <http://marc.theaimsgroup.com/?l=linux-ia64&m=109709521721980&w=2>
>>
>> I'm ok with it too.
>>
>> hth,
>> grant
>> -
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>We came to a resolution on the pci_root-ops issue, Jesse is OK
>with the code, Jes and Christoph are
>fine with the qla mod. I've added a couple of fixes from us as
>well as removing a redundant check
>pointed out in the review - see the full list below. So the
>code is ready to go.
>
>Can you take this now Tony ?
Yes. I applied those seven changesets to an local tree. If any other
bug fixes turn up between now and 2.6.9-final you can send an 008-patch
that sits on top of those.
Yesterday on LKML Linus says he's aiming for 2.6.9 in a week or so.
When that happens I'll pull this into my tree and push to him (I don't
want to put it into my bkbits tree yet ... just in case I have some
other critical patch for ia64 that needs to go into 2.6.9).
-Tony