* Altix I/O code reorganization
@ 2004-08-04 20:14 Pat Gefre
2004-08-05 0:31 ` Grant Grundler
` (2 more replies)
0 siblings, 3 replies; 36+ messages in thread
From: Pat Gefre @ 2004-08-04 20:14 UTC (permalink / raw)
To: linux-ia64; +Cc: linux-kernel
We have reorganized the I/O layer in the Altix code.
We are posting this code for review before submitting for inclusion in
the 2.5 tree.
The patch set is at: ftp://oss.sgi.com/projects/sn2/sn2-update/
It is based off the http://lia64.bkbits.net/to-linus-2.5 tree
The general changes are:
o added new hardware support
o code cleanup (typedefs, include files, etc.)
o simplified the directory structure (all files were arch/ia64/sn/io/
are now under arch/ia64/sn/ioif/)
o code size reduced by >50%
o major reorg of the code itself
o copyright updates
The patches and a short comment for each:
001-deleted-files
# contains all the files that are no longer needed
002-pci-fixup
# The io_init.c file replaces the pci_bus_cvlink.c. A diff of the files
# would not make much sense as the functionalities provided in pci_bus_cvlink.c
# are no longer needed. The new functions needed are:
#
# 1. Getting Platform Specific Information for PCI Buses/Devices.
# 2. Getting Hardware Workaround Information.
# 3. Getting I/O Hub Information.
#
# The io_init.c file basically contains all the routines that are needed to
# allow a PCI Device Driver to perform Interrupts, PIOs and DMAs.
003-pci-support
# There are some minor changes in these files. The biggest change is that we have
# broken up the SN Platform Specific DMA mapping interfaces into 3 different
# routines:
#
# 1. 32Bit Direct Mapping.
# 2. 32Bit PMU Mapping.
# 3. 64Bit Direct Mapping.
#
# The SN Platform has 3 different PCI/PCIX Bridges. They are not all exactly the
# same, however they do provide the same functionality. We have abstracted
# the DMA mapping calls so that the caller will not have to be aware of the
# hardware differences. Example:
#
# *dma_handle = (*provider->dmatrans_direct64)(pcidev_info, phys_addr,
# PCIIO_DMA_CMD | PCIIO_DMA_A64);
#
# The Bus Driver for this card, determines the appropriate method.
#
004-pci-bridge-drivers
# This set of new files supports 2 of our 3 PCI Bridges, PIC and TIOCP.
# pcibr_dma.c is new and provides the PCI DMA Mapping routines.
# pcibr_sal_interfaces.c is new and provides the SAL Interfaces.
# pcibr_provider.c is new and defines the Chipset specific Provider Tables.
#
# The rest of the file changes are mostly cleanup.
005-klconfig
# Moved a file, but mostly cleanup
006-bte
# Changes needed to remove hwgraph
007-io-hub-provider
# Files needed to provide services for our I/O Hub devices
008-kdb-support-functions
# KDB support functions
009-misc-sources
# code cleanup
# ran thru Lindent
# changes needed for new I/O structure
010-new-includes
# New includes with definitions for PCI devices, buses, and I/O hubs
011-misc-include-changes
# code cleanup
# misc changes needed for new I/O structure
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-04 20:14 Altix I/O code reorganization Pat Gefre
@ 2004-08-05 0:31 ` Grant Grundler
2004-08-05 18:16 ` Greg KH
2004-08-06 13:18 ` Christoph Hellwig
2 siblings, 0 replies; 36+ messages in thread
From: Grant Grundler @ 2004-08-05 0:31 UTC (permalink / raw)
To: Pat Gefre; +Cc: linux-ia64, linux-kernel
On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
> We are posting this code for review before submitting for inclusion in
> the 2.5 tree.
I'm sure you really meant 2.6.
> The patch set is at: ftp://oss.sgi.com/projects/sn2/sn2-update/
>
> It is based off the http://lia64.bkbits.net/to-linus-2.5 tree
I was previously told to not use that tree.
It's only for linus to pull a subset of patches from.
Tony, is "http://lia64.bkbits.net:8080/linux-ia64-2.56" the right
tree to use?
hth,
grant
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-04 20:14 Altix I/O code reorganization Pat Gefre
2004-08-05 0:31 ` Grant Grundler
@ 2004-08-05 18:16 ` Greg KH
2004-08-05 20:51 ` Pat Gefre
2004-08-06 13:18 ` Christoph Hellwig
2 siblings, 1 reply; 36+ messages in thread
From: Greg KH @ 2004-08-05 18:16 UTC (permalink / raw)
To: Pat Gefre; +Cc: linux-ia64, linux-kernel
On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
>
> The patches and a short comment for each:
Care to post the patches here so that we can comment on them?
greg k-h
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-05 18:16 ` Greg KH
@ 2004-08-05 20:51 ` Pat Gefre
2004-08-05 21:08 ` Greg KH
0 siblings, 1 reply; 36+ messages in thread
From: Pat Gefre @ 2004-08-05 20:51 UTC (permalink / raw)
To: Greg KH; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Thu, 5 Aug 2004, Greg KH wrote:
+ On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
+ >
+ > The patches and a short comment for each:
+
+ Care to post the patches here so that we can comment on them?
+
+ greg k-h
+
I thought I put the url where the patches are.
You should be able to see them here:
ftp://oss.sgi.com/projects/sn2/sn2-update/
-- Pat
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-05 20:51 ` Pat Gefre
@ 2004-08-05 21:08 ` Greg KH
2004-08-05 21:32 ` Jesse Barnes
0 siblings, 1 reply; 36+ messages in thread
From: Greg KH @ 2004-08-05 21:08 UTC (permalink / raw)
To: Pat Gefre; +Cc: linux-ia64, linux-kernel
On Thu, Aug 05, 2004 at 03:51:46PM -0500, Pat Gefre wrote:
> On Thu, 5 Aug 2004, Greg KH wrote:
>
> + On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
> + >
> + > The patches and a short comment for each:
> +
> + Care to post the patches here so that we can comment on them?
> +
> + greg k-h
> +
>
> I thought I put the url where the patches are.
You did. You must not have read what Documentation/SubmittingPatches
says to do...
greg k-h
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-05 21:08 ` Greg KH
@ 2004-08-05 21:32 ` Jesse Barnes
2004-08-05 21:36 ` Greg KH
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2004-08-05 21:32 UTC (permalink / raw)
To: Greg KH; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Thursday, August 5, 2004 2:08 pm, Greg KH wrote:
> On Thu, Aug 05, 2004 at 03:51:46PM -0500, Pat Gefre wrote:
> > On Thu, 5 Aug 2004, Greg KH wrote:
> >
> > + On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
> > + >
> > + > The patches and a short comment for each:
> > +
> > + Care to post the patches here so that we can comment on them?
> > +
> > + greg k-h
> > +
> >
> > I thought I put the url where the patches are.
>
> You did. You must not have read what Documentation/SubmittingPatches
> says to do...
Quoting said document:
7) E-mail size.
When sending patches to Linus, always follow step #6.
Large changes are not appropriate for mailing lists, and some
maintainers. If your patch, uncompressed, exceeds 40 kB in size,
it is preferred that you store your patch on an Internet-accessible
server, and provide instead a URL (link) pointing to your patch.
A few of the patches at that URL are very large (one is over 400k). That
said, some of the others could probably be posted.
Jesse
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-05 21:32 ` Jesse Barnes
@ 2004-08-05 21:36 ` Greg KH
0 siblings, 0 replies; 36+ messages in thread
From: Greg KH @ 2004-08-05 21:36 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Thu, Aug 05, 2004 at 02:32:36PM -0700, Jesse Barnes wrote:
>
> A few of the patches at that URL are very large (one is over 400k). That
> said, some of the others could probably be posted.
Exactly. They _should_ be posted.
greg k-h
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-04 20:14 Altix I/O code reorganization Pat Gefre
2004-08-05 0:31 ` Grant Grundler
2004-08-05 18:16 ` Greg KH
@ 2004-08-06 13:18 ` Christoph Hellwig
2004-08-06 13:51 ` Keith Owens
` (3 more replies)
2 siblings, 4 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-06 13:18 UTC (permalink / raw)
To: Pat Gefre; +Cc: linux-ia64, linux-kernel
On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
> o added new hardware support
> o code cleanup (typedefs, include files, etc.)
> o simplified the directory structure (all files were arch/ia64/sn/io/
> are now under arch/ia64/sn/ioif/)
> o code size reduced by >50%
> o major reorg of the code itself
> o copyright updates
Yikes, this is truely horrible. First your patch ordering doesn't make
any sense, with just the first patch applied the system won't work at all.
Please submit a series of _small_ patches going from A to B keeping the code
working everywhere inbetween.
Your new directory structure is very bad. Just stick all files into
arch/ia64/sn/io/ instead of adding subdirectories for often just a single
file.
Now to the contents:
002-pci-fixups:
you're adding tons of non-standard SAL calls for who knows what. In
fact this pretty much looks like you're just moving the existing crappy
code into the prom so the bad Linux guys can't complain about it anymore.
Please switch to the standard ACPI PCI probing mechanism all other IA64
machines support and you can get rid of all that.
You're duplicating the kernel's PCI to PCI bridge support, with the normal
IA64 pci code it would just work..
003-pci-support:
The PCI DMA implementation is still ubercomplicated. See the PCI DMA code
I sent you long ago.
Of the code in pci_extensions.c only two are actually used in the kernel
(snia_pcibr_rrb_alloc and snia_pcidev_endian_set), please remove the unused
other ones.
004-pci-bridge_drivers:
You still have code dealing with all kinds of PCIIO_ and PCIBR_ flags
that will never be set through the Linux interfaces. Again see the DMA
mapping code I sent you.
006-bte:
Please merge bte_error.c into the existing bte.c
007-io-hub-provider:
tio_provider and hub_provider have exactly the same methods, no need to
keep the xtalk_provider_t abstraction at all
008-kdb-support-funtions:
kdb isn't in mainline, please add the two files to the kdb patch instead
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 13:18 ` Christoph Hellwig
@ 2004-08-06 13:51 ` Keith Owens
2004-08-06 13:55 ` Christoph Hellwig
2004-08-06 15:47 ` Russ Anderson
` (2 subsequent siblings)
3 siblings, 1 reply; 36+ messages in thread
From: Keith Owens @ 2004-08-06 13:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Fri, 6 Aug 2004 14:18:36 +0100,
Christoph Hellwig <hch@infradead.org> wrote:
>008-kdb-support-funtions:
> kdb isn't in mainline, please add the two files to the kdb patch instead
No. We have had this discussion before - kdb is an extensible
debugger. Subsystems can add their own kdb commands to decode their
own data. Those extensions to kdb belong in the subsystem code, not in
the main kdb patch.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 13:51 ` Keith Owens
@ 2004-08-06 13:55 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-06 13:55 UTC (permalink / raw)
To: Keith Owens; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Fri, Aug 06, 2004 at 11:51:54PM +1000, Keith Owens wrote:
> No. We have had this discussion before - kdb is an extensible
> debugger. Subsystems can add their own kdb commands to decode their
> own data. Those extensions to kdb belong in the subsystem code, not in
> the main kdb patch.
They do not belong into mainline. kdb isn't in mainline and we shouldn't
carry code for it around. I don't care whether you want it in the kdb patch
or whatether it's in a separate one.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 13:18 ` Christoph Hellwig
2004-08-06 13:51 ` Keith Owens
@ 2004-08-06 15:47 ` Russ Anderson
2004-08-06 16:19 ` Jesse Barnes
2004-08-11 23:24 ` Patrick Gefre
3 siblings, 0 replies; 36+ messages in thread
From: Russ Anderson @ 2004-08-06 15:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Pat Gefre, linux-ia64, linux-kernel
Christoph Hellwig wrote:
>
> 006-bte:
> Please merge bte_error.c into the existing bte.c
bte_crb_error_handler() is called from hubiio_crb_error_handler()
in arch/ia64/sn/io/sn2/shuberror.c. That's why bte_error.c is
in the same directory as shuberror.c (and not in arch/ia64/sn/kernel
with bte.c).
If bte_error.c is merged, wouldn't it make more sense to
merge it with shuberror.c?
Thanks,
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 13:18 ` Christoph Hellwig
2004-08-06 13:51 ` Keith Owens
2004-08-06 15:47 ` Russ Anderson
@ 2004-08-06 16:19 ` Jesse Barnes
2004-08-07 10:58 ` Christoph Hellwig
2004-08-11 23:24 ` Patrick Gefre
3 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2004-08-06 16:19 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Friday, August 6, 2004 6:18 am, Christoph Hellwig wrote:
> Yikes, this is truely horrible. First your patch ordering doesn't make
> any sense, with just the first patch applied the system won't work at all.
> Please submit a series of _small_ patches going from A to B keeping the
> code working everywhere inbetween.
Much of this stuff is clearly interdependent (and dependent on PROM changes),
so I don't think that would make sense.
Jesse
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 16:19 ` Jesse Barnes
@ 2004-08-07 10:58 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-07 10:58 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Pat Gefre, linux-ia64, linux-kernel
On Fri, Aug 06, 2004 at 09:19:20AM -0700, Jesse Barnes wrote:
> On Friday, August 6, 2004 6:18 am, Christoph Hellwig wrote:
> > Yikes, this is truely horrible. First your patch ordering doesn't make
> > any sense, with just the first patch applied the system won't work at all.
> > Please submit a series of _small_ patches going from A to B keeping the
> > code working everywhere inbetween.
>
> Much of this stuff is clearly interdependent (and dependent on PROM changes),
> so I don't think that would make sense.
It's not. E.g. hwgraph removal and dma code rework aren't related to hiding
of lots of interfaces in the firmware at all.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-06 13:18 ` Christoph Hellwig
` (2 preceding siblings ...)
2004-08-06 16:19 ` Jesse Barnes
@ 2004-08-11 23:24 ` Patrick Gefre
2004-08-12 9:15 ` Christoph Hellwig
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
3 siblings, 2 replies; 36+ messages in thread
From: Patrick Gefre @ 2004-08-11 23:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ia64, linux-kernel
I'm sending out a new patch set - the set of files will follow this email.
I'll add in this email the comments on the general comments and the comments on
the specific pach comments will be in the email for that updated patch - hopefully
that makes some kind of sense 8^). I didn't include the small bte change in this
set.
Christoph Hellwig wrote:
> On Wed, Aug 04, 2004 at 03:14:08PM -0500, Pat Gefre wrote:
>
>
> Yikes, this is truely horrible. First your patch ordering doesn't make
> any sense, with just the first patch applied the system won't work at all.
> Please submit a series of _small_ patches going from A to B keeping the code
> working everywhere inbetween.
>
This is a very BIG change. However, the BIG change ends up with
very little code in the kernel. The reason is because, we have enhanced the
functionalities in our Prom to actually configure and initialize all devices
in the system instead of just the BaseIO devices.
It is not practical to have small patches that will work independently.
The code base is now small enough that we should not have a problem
providing feedback.
> Your new directory structure is very bad. Just stick all files into
> arch/ia64/sn/io/ instead of adding subdirectories for often just a single
> file.
>
We do like our directory structures. It provides very logical
separation of code files.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-11 23:24 ` Patrick Gefre
@ 2004-08-12 9:15 ` Christoph Hellwig
2004-08-12 14:47 ` Jesse Barnes
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
1 sibling, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-12 9:15 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Wed, Aug 11, 2004 at 06:24:43PM -0500, Patrick Gefre wrote:
> This is a very BIG change. However, the BIG change ends up with
> very little code in the kernel. The reason is because, we have enhanced the
> functionalities in our Prom to actually configure and initialize all devices
> in the system instead of just the BaseIO devices.
Surely it can be spit out. E.g. hwgraph removal is separate from your
SAL call stuff which is separate from dma mapping.
And, let me repeat:
There is absolutely _NO_ interest in adding yet another non-standard
prom interface for PCI configuration. IA64 has a standard ACPI-based
interface that everyone but SGI implementent. Please implement that one
in your firmware.
> We do like our directory structures. It provides very logical
> separation of code files.
You have more subdirectories than everything else in arch/ia64/ combined,
go figure.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-12 9:15 ` Christoph Hellwig
@ 2004-08-12 14:47 ` Jesse Barnes
2004-08-12 15:21 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Jesse Barnes @ 2004-08-12 14:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Patrick Gefre, linux-ia64, linux-kernel
On Thursday, August 12, 2004 2:15 am, Christoph Hellwig wrote:
> And, let me repeat:
>
> There is absolutely _NO_ interest in adding yet another non-standard
> prom interface for PCI configuration. IA64 has a standard ACPI-based
> interface that everyone but SGI implementent. Please implement that
> one in your firmware.
Our platform does not currently support ACPI based PCI discovery and
configuration. My claim is that this patchset gets us closer to being able
to implement it. You aren't saying that any changes to the codebase should
be rejected until ACPI is 100% supported, are you? That seems like a silly
stance to take since it precludes incremental improvements in the codebase.
IOW, assuming the patchset meets with general approval in other ways, you
wouldn't oppose its inclusion just because it doesn't go far enough, would
you?
Thanks,
Jesse
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Altix I/O code reorganization
2004-08-12 14:47 ` Jesse Barnes
@ 2004-08-12 15:21 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-12 15:21 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Christoph Hellwig, Patrick Gefre, linux-ia64, linux-kernel
On Thu, Aug 12, 2004 at 07:47:02AM -0700, Jesse Barnes wrote:
> Our platform does not currently support ACPI based PCI discovery and
> configuration. My claim is that this patchset gets us closer to being able
> to implement it. You aren't saying that any changes to the codebase should
> be rejected until ACPI is 100% supported, are you? That seems like a silly
> stance to take since it precludes incremental improvements in the codebase.
Changes to the codebase are just fine. Adding new interface to the prom
the duplicate the standard interface for that prupose on IA64 are not fine.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Latest Altix I/O code reorganization code
2004-08-11 23:24 ` Patrick Gefre
2004-08-12 9:15 ` Christoph Hellwig
@ 2004-08-27 15:10 ` Patrick Gefre
2004-08-27 15:14 ` Patrick Gefre
` (4 more replies)
1 sibling, 5 replies; 36+ messages in thread
From: Patrick Gefre @ 2004-08-27 15:10 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
This is an update to our last set of patches. I've added some comments from the
last review and another synopsis of the patches. The individual patches will
follow this email.
Comments from the last review:
Christoph Hellwig <hch@infradead.org> wrote:
>>
>> The PCIIO_ flags are designed for usage at the Device Driver layer,
>> while the PCIBR_ flags are designed to be the actual hardware bits that needed
>> setting. However, with the latest code, we have deleted the PCIIO_ and
>> PCIBR_ flags that are not used.
>
>
> There still are tons that are used in the code, but never logically set from
> the exported interfaces.
Should be gone.
>> and tries to export
>> the many hardware features that are available on our system. However, not
>> all of these features are "reachable" via the standard Linux PCI mapping
>> interfaces. We have removed all unreacheable features. However, we can
>> still support future requirements with these new interfaces when needed.
>>
>> Yes, we did look at your patch dated: December 4th 2003. It was very good, but
>
>
> I mean a more recent patch I sent to Colin.
That patch contains xbridge structs which don't belong in our 2.6 code.
>+xtalk_provider_t hub_provider = {
>> +
>> + (xtalk_intr_alloc_f *) sal_xtalk_intr_alloc,
>> + (xtalk_intr_free_f *) sal_xtalk_intr_free,
>> +
>> +};
>> +
>> +xtalk_provider_t tio_provider = {
>> +
>> + (xtalk_intr_alloc_f *) sal_xtalk_intr_alloc,
>> + (xtalk_intr_free_f *) sal_xtalk_intr_free,
>> +
>> +};
>
>
> you still have it in this patch.
Should be gone - the last patch was my fault - I had grabbed an older file.
We also flattened our directory structure:
$ ls -R arch/ia64/sn/ioif
arch/ia64/sn/ioif:
io_init.c iomv.c Makefile pci pci_dma.c pci_extension.c
arch/ia64/sn/ioif/pci:
Makefile pcibr_ate.c pcibr_dma.c pcibr_provider.c pcibr_reg.c xtalk_providers.c
Note the diffstat output - we are purging a lot of code:
114 files changed, 4679 insertions(+), 46952 deletions(-)
We made some changes to our SAL call interface - once we get ACPI going,
we will remove all these SAL calls. EFI 1.10 and ACPI compliance will
be the next 2 phases in our development.
Patch synopsis:
We have reorganized the I/O layer in the Altix code.
We are posting this code for review before submitting for inclusion in
the 2.6 tree.
The patch set is at: ftp://oss.sgi.com/projects/sn2/sn2-update/
It is based off http://lia64.bkbits.net/linux-ia64-test-2.6.8.1
The general changes 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.
diffstat of all patches:
arch/ia64/sn/io/Makefile | 13
arch/ia64/sn/io/cdl.c | 79
arch/ia64/sn/io/drivers/Makefile | 10
arch/ia64/sn/io/drivers/ioconfig_bus.c | 382
arch/ia64/sn/io/hwgfs/Makefile | 10
arch/ia64/sn/io/hwgfs/hcl.c | 702 -
arch/ia64/sn/io/hwgfs/hcl_util.c | 175
arch/ia64/sn/io/hwgfs/interface.c | 325
arch/ia64/sn/io/hwgfs/labelcl.c | 656 -
arch/ia64/sn/io/hwgfs/ramfs.c | 208
arch/ia64/sn/io/io.c | 739 -
arch/ia64/sn/io/machvec/Makefile | 10
arch/ia64/sn/io/machvec/iomv.c | 76
arch/ia64/sn/io/machvec/pci.c | 52
arch/ia64/sn/io/machvec/pci_bus_cvlink.c | 909 -
arch/ia64/sn/io/machvec/pci_dma.c | 677 -
arch/ia64/sn/io/platform_init/Makefile | 10
arch/ia64/sn/io/platform_init/sgi_io_init.c | 174
arch/ia64/sn/io/sn2/Makefile | 14
arch/ia64/sn/io/sn2/bte_error.c | 217
arch/ia64/sn/io/sn2/geo_op.c | 311
arch/ia64/sn/io/sn2/klconflib.c | 572
arch/ia64/sn/io/sn2/klgraph.c | 577
arch/ia64/sn/io/sn2/l1_command.c | 131
arch/ia64/sn/io/sn2/ml_SN_init.c | 109
arch/ia64/sn/io/sn2/ml_SN_intr.c | 320
arch/ia64/sn/io/sn2/ml_iograph.c | 770 -
arch/ia64/sn/io/sn2/module.c | 236
arch/ia64/sn/io/sn2/pcibr/Makefile | 16
arch/ia64/sn/io/sn2/pcibr/pcibr_ate.c | 178
arch/ia64/sn/io/sn2/pcibr/pcibr_config.c | 195
arch/ia64/sn/io/sn2/pcibr/pcibr_dvr.c | 2662 ----
arch/ia64/sn/io/sn2/pcibr/pcibr_error.c | 1873 ---
arch/ia64/sn/io/sn2/pcibr/pcibr_hints.c | 175
arch/ia64/sn/io/sn2/pcibr/pcibr_intr.c | 700 -
arch/ia64/sn/io/sn2/pcibr/pcibr_reg.c | 879 -
arch/ia64/sn/io/sn2/pcibr/pcibr_rrb.c | 887 -
arch/ia64/sn/io/sn2/pcibr/pcibr_slot.c | 1842 ---
arch/ia64/sn/io/sn2/pciio.c | 1004 -
arch/ia64/sn/io/sn2/pic.c | 835 -
arch/ia64/sn/io/sn2/shub.c | 246
arch/ia64/sn/io/sn2/shub_intr.c | 259
arch/ia64/sn/io/sn2/shuberror.c | 822 -
arch/ia64/sn/io/sn2/shubio.c | 490
arch/ia64/sn/io/sn2/xbow.c | 1020 -
arch/ia64/sn/io/sn2/xtalk.c | 927 -
arch/ia64/sn/io/snia_if.c | 108
arch/ia64/sn/io/xswitch.c | 168
include/asm-ia64/sn/pci/bridge.h | 1895 ---
include/asm-ia64/sn/pci/pci_bus_cvlink.h | 70
include/asm-ia64/sn/pci/pci_defs.h | 414
include/asm-ia64/sn/pci/pcibr.h | 535
include/asm-ia64/sn/pci/pcibr_private.h | 811 -
include/asm-ia64/sn/pci/pciio.h | 746 -
include/asm-ia64/sn/pci/pciio_private.h | 145
include/asm-ia64/sn/sn2/shub_md.h | 275
include/asm-ia64/sn/sn2/shub_mmr_t.h |14829 -------------------------
include/asm-ia64/sn/sn2/sn_private.h | 245
include/asm-ia64/sn/xtalk/xswitch.h | 56
include/asm-ia64/sn/xtalk/xtalk.h | 360
include/asm-ia64/sn/xtalk/xtalk_private.h | 79
include/asm-ia64/sn/xtalk/xtalkaddrs.h | 106
include/asm-ia64/sn/xtalk/xwidget.h | 240
arch/ia64/sn/Makefile | 2
arch/ia64/sn/ioif/Makefile | 16
arch/ia64/sn/ioif/io_init.c | 438
arch/ia64/sn/ioif/iomv.c | 76
arch/ia64/sn/ioif/pci/Makefile | 12
arch/ia64/sn/ioif/pci/pcibr_ate.c | 187
arch/ia64/sn/ioif/pci/pcibr_dma.c | 305
arch/ia64/sn/ioif/pci/pcibr_provider.c | 225
arch/ia64/sn/ioif/pci/pcibr_reg.c | 332
arch/ia64/sn/ioif/pci/xtalk_providers.c | 50
arch/ia64/sn/ioif/pci_dma.c | 573
arch/ia64/sn/ioif/pci_extension.c | 33
arch/ia64/sn/kernel/irq.c | 436
arch/ia64/sn/kernel/setup.c | 333
include/asm-ia64/sn/addrs.h | 23
include/asm-ia64/sn/arch.h | 5
include/asm-ia64/sn/geo.h | 31
include/asm-ia64/sn/hcl.h | 52
include/asm-ia64/sn/intr.h | 2
include/asm-ia64/sn/io.h | 2
include/asm-ia64/sn/iograph.h | 26
include/asm-ia64/sn/klconfig.h | 208
include/asm-ia64/sn/ksys/l1.h | 4
include/asm-ia64/sn/module.h | 55
include/asm-ia64/sn/nodepda.h | 60
include/asm-ia64/sn/pci/pcibr_provider.h | 188
include/asm-ia64/sn/pci/pcibus_provider_defs.h | 120
include/asm-ia64/sn/pci/pcidev.h | 58
include/asm-ia64/sn/pci/pic.h | 360
include/asm-ia64/sn/pci/tiocp.h | 256
include/asm-ia64/sn/pda.h | 3
include/asm-ia64/sn/router.h | 7
include/asm-ia64/sn/sgi.h | 23
include/asm-ia64/sn/sn2/addrs.h | 58
include/asm-ia64/sn/sn2/arch.h | 3
include/asm-ia64/sn/sn2/geo.h | 7
include/asm-ia64/sn/sn2/intr.h | 29
include/asm-ia64/sn/sn2/shub.h | 3
include/asm-ia64/sn/sn2/shubio.h | 1669 +-
include/asm-ia64/sn/sn2/tio.h | 45
include/asm-ia64/sn/sn_cpuid.h | 8
include/asm-ia64/sn/sn_sal.h | 63
include/asm-ia64/sn/sndrv.h | 1
include/asm-ia64/sn/types.h | 2
include/asm-ia64/sn/xtalk/corelet.h | 22
include/asm-ia64/sn/xtalk/hubdev.h | 61
include/asm-ia64/sn/xtalk/xbow.h | 456
include/asm-ia64/sn/xtalk/xbow_info.h | 2
include/asm-ia64/sn/xtalk/xtalk_provider.h | 52
include/asm-ia64/sn/xtalk/xtalk_sal.h | 20
include/asm-ia64/sn/xtalk/xwidgetdev.h | 73
114 files changed, 4679 insertions(+), 46952 deletions(-)
The patches and a short comment for each:
001-kill-files
002-kill-files
003-kill-files
004-kill-files
005-kill-files
# contains deleted files
006-pci-fixup
# The io_init.c file replaces the pci_bus_cvlink.c. A diff of the files
# would not make much sense as the functionalities provided in pci_bus_cvlink.c
# are no longer needed. The new functions needed are:
#
# 1. Getting Platform Specific Information for PCI Buses/Devices.
# 2. Getting Hardware Workaround Information.
# 3. Getting I/O Hub Information.
#
# The io_init.c file basically contains all the routines that are needed to
# allow a PCI Device Driver to perform Interrupts, PIOs and DMAs.
007-pci-support
# There are some minor changes in these files. The biggest change is that we have
# broken up the SN Platform Specific DMA mapping interfaces into 3 different
# routines:
#
# 1. 32Bit Direct Mapping.
# 2. 32Bit PMU Mapping.
# 3. 64Bit Direct Mapping.
#
# The SN Platform has 3 different PCI/PCIX Bridges. They are not all exactly the
# same, however they do provide the same functionality. We have abstracted
# the DMA mapping calls so that the caller will not have to be aware of the
# hardware differences. Example:
#
# *dma_handle = (*provider->dmatrans_direct64)(pcidev_info, phys_addr,
# PCIIO_DMA_CMD | PCIIO_DMA_A64);
#
# The Bus Driver for this card, determines the appropriate method.
#
008-pci-bridge-drivers-new
009-pci-bridge-drivers-kill
010-pci-bridge-drivers-new
# This set of new files supports 2 of our 3 PCI Bridges, PIC and TIOCP.
011-misc-sources
# code cleanup
# ran thru Lindent
# changes needed for new I/O structure
012-new-includes-pci
013-new-includes-xtalk
# New includes with definitions for PCI devices, buses, and I/O hubs
014-include-changes
015-include-changes
016-include-changes
017-include-changes
# code cleanup
# misc changes needed for new I/O structure
018-new-include-tio
# New include for TIO defs
019-kill-hwgraph
# kill the hwgraph code
020-kill-files
# more file zapping
021-Makefile-cleanup
# fix upper level Makefile for new directory
022-Makefile-killing
023-pci-include-killing
024-sn2-include-killing
025-new-include-tiocp
026-xtalk-include-killing
# more file purging - add one new include for tiocp
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
@ 2004-08-27 15:14 ` Patrick Gefre
2004-08-27 15:21 ` Christoph Hellwig
2004-08-27 15:36 ` Latest Altix I/O code reorganization code Christoph Hellwig
` (3 subsequent siblings)
4 siblings, 1 reply; 36+ messages in thread
From: Patrick Gefre @ 2004-08-27 15:14 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
Patch 1 of 26
This patch is too large for email see:
ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:14 ` Patrick Gefre
@ 2004-08-27 15:21 ` Christoph Hellwig
2004-08-27 15:35 ` Patrick Gefre
0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 15:21 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:14:23AM -0500, Patrick Gefre wrote:
>
> Patch 1 of 26
>
> This patch is too large for email see:
>
> ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
Stoop again. This absolutely does not look like a change after which the
tree build, right? After how many of the patches do we get a useable
kernel for SN2? Please split your changes into LOGICAL steps, e.g.
- dma mapping rework
- usage of SAL interfaces
- hwgrpah removal
- renaming of files without code changes
- addition of new hardware support
- etc..
everything else is simply not reviewable. (and the above is intentionally
an order which I think would make sense)
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:21 ` Christoph Hellwig
@ 2004-08-27 15:35 ` Patrick Gefre
2004-08-27 15:44 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Gefre @ 2004-08-27 15:35 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ia64, linux-kernel
Christoph Hellwig wrote:
> On Fri, Aug 27, 2004 at 10:14:23AM -0500, Patrick Gefre wrote:
>
>>Patch 1 of 26
>>
>>This patch is too large for email see:
>>
>>ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files
>
>
>
> Stoop again. This absolutely does not look like a change after which the
> tree build, right? After how many of the patches do we get a useable
Right
> kernel for SN2? Please split your changes into LOGICAL steps, e.g.
26 - hence the 1 of 26
>
> - dma mapping rework
> - usage of SAL interfaces
> - hwgrpah removal
> - renaming of files without code changes
> - addition of new hardware support
> - etc..
>
The code was completely redone. It was not done in sections. Breaking them into
pieces is nothing more than an academic exercise. It is not useable until all
the code changes have been made because it would require a prom that followed these
changes. The point of a review was to get comments on the CONTENT of the code not
the format of the patches.
> everything else is simply not reviewable. (and the above is intentionally
> an order which I think would make sense)
The easiest way to review is to delete the old directorory and apply the new ioif
files. Reviewing it as patches doesn't make any sense. The new code is very small and
it should be possible to look at it intact.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
2004-08-27 15:14 ` Patrick Gefre
@ 2004-08-27 15:36 ` Christoph Hellwig
2004-08-27 15:45 ` Christoph Hellwig
` (2 subsequent siblings)
4 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 15:36 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
> io_init.c iomv.c Makefile pci pci_dma.c pci_extension.c
>
> arch/ia64/sn/ioif/pci:
> Makefile pcibr_ate.c pcibr_dma.c pcibr_provider.c pcibr_reg.c xtalk_providers.c
>
So why are pci_dma.c and pci_extensions.c not under pci? You probably should
just kill that pci subdir, too when there's just two other files left. Or
move those two to kernel/ and have a pci/ subdir which seems to fit other
ports quite well. But I'd rather see the functional changes first and then
do renaming of unchanged files last anyway so you changed get actually
reviewable. The current patchkit is not.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:35 ` Patrick Gefre
@ 2004-08-27 15:44 ` Christoph Hellwig
2004-09-03 23:12 ` Latest Altix I/O code rewrite Patrick Gefre
0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 15:44 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:35:48AM -0500, Patrick Gefre wrote:
> The code was completely redone. It was not done in sections. Breaking them into
> pieces is nothing more than an academic exercise. It is not useable until all
> the code changes have been made because it would require a prom that followed these
> changes. The point of a review was to get comments on the CONTENT of the code not
> the format of the patches.
This might be true for some parts, for others it certainy isn't. E.g.
pci dma code still loooks remarkably similar to the old one, the klconfig
code is only stripped down a little, there's quite a few changes in the
headers that don't make much sense.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
2004-08-27 15:14 ` Patrick Gefre
2004-08-27 15:36 ` Latest Altix I/O code reorganization code Christoph Hellwig
@ 2004-08-27 15:45 ` Christoph Hellwig
2004-08-27 16:32 ` Patrick Gefre
2004-08-27 15:54 ` Christoph Hellwig
2004-08-27 17:15 ` Christoph Hellwig
4 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 15:45 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
>
> This is an update to our last set of patches. I've added some comments from the
> last review and another synopsis of the patches. The individual patches will
> follow this email.
Applying this against current BK gives a reject in
arch/ia64/sn/io/machvec/pci_bus_cvlink.c for the 6th patch
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
` (2 preceding siblings ...)
2004-08-27 15:45 ` Christoph Hellwig
@ 2004-08-27 15:54 ` Christoph Hellwig
2004-08-27 16:06 ` Patrick Gefre
2004-08-27 16:21 ` Christoph Hellwig
2004-08-27 17:15 ` Christoph Hellwig
4 siblings, 2 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 15:54 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
>
> This is an update to our last set of patches. I've added some comments from the
> last review and another synopsis of the patches. The individual patches will
> follow this email.
Matthew Wilcox suggested we should really review it as completely new
code. So maybe you should split it into one patch that kills all old
code ånd one that adds new code. Note that I mean all all code includeing
the headers which provides a nice opporuntiy to move all of them that
aren't public interface inside arch/ia64/sn/
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:54 ` Christoph Hellwig
@ 2004-08-27 16:06 ` Patrick Gefre
2004-08-27 16:21 ` Christoph Hellwig
1 sibling, 0 replies; 36+ messages in thread
From: Patrick Gefre @ 2004-08-27 16:06 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ia64, linux-kernel
Christoph Hellwig wrote:
> On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
>
>>This is an update to our last set of patches. I've added some comments from the
>>last review and another synopsis of the patches. The individual patches will
>>follow this email.
>
>
> Matthew Wilcox suggested we should really review it as completely new
> code. So maybe you should split it into one patch that kills all old
> code end one that adds new code. Note that I mean all all code includeing
> the headers which provides a nice opporuntiy to move all of them that
> aren't public interface inside arch/ia64/sn/
>
This makes sense. I'll start working on this right away.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:54 ` Christoph Hellwig
2004-08-27 16:06 ` Patrick Gefre
@ 2004-08-27 16:21 ` Christoph Hellwig
2004-08-29 6:39 ` Keith Owens
2004-09-03 23:40 ` Christoph Hellwig
1 sibling, 2 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 16:21 UTC (permalink / raw)
To: Patrick Gefre, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 04:54:43PM +0100, Christoph Hellwig wrote:
> On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
> >
> > This is an update to our last set of patches. I've added some comments from the
> > last review and another synopsis of the patches. The individual patches will
> > follow this email.
>
> Matthew Wilcox suggested we should really review it as completely new
> code. So maybe you should split it into one patch that kills all old
> code ånd one that adds new code. Note that I mean all all code includeing
> the headers which provides a nice opporuntiy to move all of them that
> aren't public interface inside arch/ia64/sn/
Okay, let's start a review for the actual files based on that
all files: lots of missing statics, try running
http://samba.org/ftp/unpacked/junkcode/findstatic.pl over the compiled code.
io_init.c:
- sal_get_hubdev_info
umm, you're getting a kernel pointer by a SAL
call. I wonders how this is supposed to work when the kernel
changes it's VA layout. (Dito for a bunch of other functions)
- sn_io_init
what's going on here?
iomv.c:
- okay, could use some reformatting to fit 80 chars
pci_dma.c:
- still using all those indirections althoug there's only a single backend
pci_extension.c:
- dito. Why does this single function need a separate file?
pcibr_ate.c:
- doesn't look to bad, but should probably merged into pcibr_dma.c. And
the trivial < 10 line function opencoded in their only callers.
pcibr_dma.c:
- okay
pcibr_provider.c
actual code code looks okay, but:
- sal_pcibr_slot_enable/sal_pcibr_slot_disabl are completely unused
- the pci_provider abstraction is right now completely useless, please
add such abstractions only when you need them (and if you'll ever need
that a few hints: stop that casting of methods silliness but use
container_of, use C99 initializers, stop the typedef abuse)
- request_irq return value needs checking
pcibr_reg.c:
- all this casting is rather horrible. At least keep a pointer to each
of struct tiocp and pic_s (and kill the _s postifx) in syruct pcibus_info.
the volatiles looks bogus, if you need it you're missing memorry barries.
xtalk_providers.c:
- bogus indirection again. if you actually do have different lowlevel
implementations they should be hidden inside the prom. While at it I think
the two calls could just move to arch/ia64/sn/kernel/irq.c
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:45 ` Christoph Hellwig
@ 2004-08-27 16:32 ` Patrick Gefre
0 siblings, 0 replies; 36+ messages in thread
From: Patrick Gefre @ 2004-08-27 16:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ia64, linux-kernel
Christoph Hellwig wrote:
> On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
>
>>This is an update to our last set of patches. I've added some comments from the
>>last review and another synopsis of the patches. The individual patches will
>>follow this email.
>
>
> Applying this against current BK gives a reject in
> arch/ia64/sn/io/machvec/pci_bus_cvlink.c for the 6th patch
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
I used the tree at http://lia64.bkbits.net/linux-ia64-test-2.6.8.1
from yesterday
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
` (3 preceding siblings ...)
2004-08-27 15:54 ` Christoph Hellwig
@ 2004-08-27 17:15 ` Christoph Hellwig
4 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-08-27 17:15 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Fri, Aug 27, 2004 at 10:10:01AM -0500, Patrick Gefre wrote:
>
> This is an update to our last set of patches. I've added some comments from the
> last review and another synopsis of the patches. The individual patches will
> follow this email.
The header changes in your patchkit are still rather strange, you're e.g.
adding a pfn_t or clusterid_t but never using it. As suggested in a previous
mail please also provide a clean set of headers in addition to the clean set
of source files. There's lots of dead wood in thos and with all the code
gone youshould be able to kill off all the IRIX-compat gunk, etc..
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 16:21 ` Christoph Hellwig
@ 2004-08-29 6:39 ` Keith Owens
2004-08-29 7:16 ` Sam Ravnborg
2004-09-03 23:40 ` Christoph Hellwig
1 sibling, 1 reply; 36+ messages in thread
From: Keith Owens @ 2004-08-29 6:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Patrick Gefre, linux-ia64, linux-kernel
On Fri, 27 Aug 2004 17:21:31 +0100,
Christoph Hellwig <hch@infradead.org> wrote:
>all files: lots of missing statics, try running
>http://samba.org/ftp/unpacked/junkcode/findstatic.pl over the compiled code.
ftp://ftp.ocs.com.au/pub/namespace.pl does a more rigorous check for
symbols that can be static. namespace.pl knows about the special
kernel symbols, linkage and EXPORT_SYMBOL().
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-29 6:39 ` Keith Owens
@ 2004-08-29 7:16 ` Sam Ravnborg
2004-08-29 7:22 ` Keith Owens
0 siblings, 1 reply; 36+ messages in thread
From: Sam Ravnborg @ 2004-08-29 7:16 UTC (permalink / raw)
To: Keith Owens; +Cc: Christoph Hellwig, Patrick Gefre, linux-ia64, linux-kernel
On Sun, Aug 29, 2004 at 04:39:34PM +1000, Keith Owens wrote:
> On Fri, 27 Aug 2004 17:21:31 +0100,
> Christoph Hellwig <hch@infradead.org> wrote:
> >all files: lots of missing statics, try running
> >http://samba.org/ftp/unpacked/junkcode/findstatic.pl over the compiled code.
>
> ftp://ftp.ocs.com.au/pub/namespace.pl does a more rigorous check for
> symbols that can be static. namespace.pl knows about the special
> kernel symbols, linkage and EXPORT_SYMBOL().
Should we add it to the support scripts in the kernel?
Something like:
make checknamespace
Sam
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-29 7:16 ` Sam Ravnborg
@ 2004-08-29 7:22 ` Keith Owens
0 siblings, 0 replies; 36+ messages in thread
From: Keith Owens @ 2004-08-29 7:22 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: Christoph Hellwig, Patrick Gefre, linux-ia64, linux-kernel
On Sun, 29 Aug 2004 09:16:35 +0200,
Sam Ravnborg <sam@ravnborg.org> wrote:
>On Sun, Aug 29, 2004 at 04:39:34PM +1000, Keith Owens wrote:
>> On Fri, 27 Aug 2004 17:21:31 +0100,
>> Christoph Hellwig <hch@infradead.org> wrote:
>> >all files: lots of missing statics, try running
>> >http://samba.org/ftp/unpacked/junkcode/findstatic.pl over the compiled code.
>>
>> ftp://ftp.ocs.com.au/pub/namespace.pl does a more rigorous check for
>> symbols that can be static. namespace.pl knows about the special
>> kernel symbols, linkage and EXPORT_SYMBOL().
>
>Should we add it to the support scripts in the kernel?
>Something like:
>
>make checknamespace
Funny about that ... I was making a patch to send to you and akpm when
your mail arrived.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Latest Altix I/O code rewrite
2004-08-27 15:44 ` Christoph Hellwig
@ 2004-09-03 23:12 ` Patrick Gefre
0 siblings, 0 replies; 36+ messages in thread
From: Patrick Gefre @ 2004-09-03 23:12 UTC (permalink / raw)
To: linux-ia64; +Cc: linux-kernel, hch
We have redone the I/O layer in the Altix code.
We are posting this code for review before submitting for inclusion in
the 2.6 tree.
I'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.
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-test-2.6.8.1
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-08-27 16:21 ` Christoph Hellwig
2004-08-29 6:39 ` Keith Owens
@ 2004-09-03 23:40 ` Christoph Hellwig
2004-09-07 22:10 ` Patrick Gefre
1 sibling, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2004-09-03 23:40 UTC (permalink / raw)
To: Christoph Hellwig, Patrick Gefre, linux-ia64, linux-kernel
Crosscheck of your new code vs the review:
> io_init.c:
>
> - sal_get_hubdev_info
> umm, you're getting a kernel pointer by a SAL
> call. I wonders how this is supposed to work when the kernel
> changes it's VA layout. (Dito for a bunch of other functions)
Still not explanation
> - sn_io_init
> what's going on here?
gone
> iomv.c:
>
> - okay, could use some reformatting to fit 80 chars
ok
> pci_dma.c:
>
> - still using all those indirections althoug there's only a single backend
this comment is fixed, but the one later sent in private (stupid choice
of interface beteen upper pci dma code and pcibr code ignored)
> pci_extension.c:
>
> - dito. Why does this single function need a separate file?
Not addressed. In general your file organization is mess still.
Just do arch/ia64/sn/pci/ for all pci code and move the rest to
arch/ia64/sn/kernel. That how most arches work.
> pcibr_ate.c:
>
> - doesn't look to bad, but should probably merged into pcibr_dma.c. And
> the trivial < 10 line function opencoded in their only callers.
Not addressed or commented on yet.
> pcibr_provider.c
>
> actual code code looks okay, but:
>
> - sal_pcibr_slot_enable/sal_pcibr_slot_disabl are completely unused
you still have typedefs for these around
> - the pci_provider abstraction is right now completely useless, please
> add such abstractions only when you need them (and if you'll ever need
> that a few hints: stop that casting of methods silliness but use
> container_of, use C99 initializers, stop the typedef abuse)
ok
> - request_irq return value needs checking
ok
>
> pcibr_reg.c:
>
> - all this casting is rather horrible. At least keep a pointer to each
> of struct tiocp and pic_s (and kill the _s postifx) in syruct pcibus_info.
> the volatiles looks bogus, if you need it you're missing memorry barries.
the type pointer isn't done. Any specific reason?
> xtalk_providers.c:
>
> - bogus indirection again. if you actually do have different lowlevel
> implementations they should be hidden inside the prom. While at it I think
> the two calls could just move to arch/ia64/sn/kernel/irq.c
ok
More items:
- sal_pcibr_rrb_alloc should be merged into its only caller
- io_init.c still has KDB ifdefs
- sn_pci_set_vchan should absolutely _not_ be placed in qla1280.c,
and while you're at it there extern for snia_pcibr_rrb_alloc should
move to a header
- kill HUBREG_CAST instead of touching it
- please don't put your ASSERT into asm/ headers. In general you
should use BUG_ON
- why do you add a pfn_t typedef that's not used anywhere
- the patch adds include/asm-ia64/sn/xtalk/xtalk_provider.h but there's
no more xtalk providers
Your headers are still a complete mess. There's no point exposing tons
of defails about the pci interface in include/asm - just keep and
pci_extensions.h there for non-standard PCI APIs, and the rest should
stay private inisde arch/ia64/sn/pci/.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-09-03 23:40 ` Christoph Hellwig
@ 2004-09-07 22:10 ` Patrick Gefre
2004-09-07 22:16 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Gefre @ 2004-09-07 22:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ia64, linux-kernel
Christoph Hellwig wrote:
> Crosscheck of your new code vs the review:
>
>
> >io_init.c:
> >
> > - sal_get_hubdev_info
> > umm, you're getting a kernel pointer by a SAL
> > call. I wonders how this is supposed to work when the kernel
> > changes it's VA layout. (Dito for a bunch of other functions)
>
>
I will change the ptr passed to a physical address.
> > - sn_io_init
> > what's going on here?
>
> gone
>
> > iomv.c:
> >
> > - okay, could use some reformatting to fit 80 chars
>
> ok
>
> > pci_dma.c:
> >
> > - still using all those indirections althoug there's only a single backend
>
> this comment is fixed, but the one later sent in private (stupid choice
> of interface beteen upper pci dma code and pcibr code ignored)
>
Guess I'm confused about this then. Are you suggesting putting the pcibr_dma files
into the pci_dma.c code and not having a pcibr_dma interface ?? The api is there because
pcibr is an ASIC and is ASIC specific.
> > pci_extension.c:
> >
> > - dito. Why does this single function need a separate file?
>
> Not addressed. In general your file organization is mess still.
>
How is this:
arch/ia64/sn/pci/
pci_dma.c
pci_extension.c
pcibr/
pcibr_ate.c
pcibr_dma.c
pcibr_provider.c
pcibr_reg.c
arch/ia64/sn/kernel/
bte_error.c
io_init.c
iomv.c
Since pcibr is an ASIC it makes sense for it to have its own directory. There are
other ASIC interfaces that will be put in in the not too distant future and they
will go in separate directories also.
> Just do arch/ia64/sn/pci/ for all pci code and move the rest to
> arch/ia64/sn/kernel. That how most arches work.
>
> > pcibr_ate.c:
> >
> > - doesn't look to bad, but should probably merged into pcibr_dma.c. And
> > the trivial < 10 line function opencoded in their only callers.
>
> Not addressed or commented on yet.
>
I will inline free_ate_resource(), alloc_ate_resource() and ate_write(). I want to
keep the ate code in a separate file - since it is a group of functions with a similar
theme and is non-trivial.
> > pcibr_provider.c
> >
> > actual code code looks okay, but:
> >
> > - sal_pcibr_slot_enable/sal_pcibr_slot_disabl are completely unused
>
> you still have typedefs for these around
>
Missed them - I'll take them out.
> > - the pci_provider abstraction is right now completely useless, please
> > add such abstractions only when you need them (and if you'll ever need
> > that a few hints: stop that casting of methods silliness but use
> > container_of, use C99 initializers, stop the typedef abuse)
>
> ok
>
> > - request_irq return value needs checking
>
> ok
>> > pcibr_reg.c:
> >
> > - all this casting is rather horrible. At least keep a pointer to each
> > of struct tiocp and pic_s (and kill the _s postifx) in syruct pcibus_info.
> > the volatiles looks bogus, if you need it you're missing memorry barries.
>
> the type pointer isn't done. Any specific reason?
>
I'm confused on this too. The struct defs are for different MMR sets - so we do need
to use different types of pointers.
> > xtalk_providers.c:
> >
> > - bogus indirection again. if you actually do have different lowlevel
> > implementations they should be hidden inside the prom. While at it I think
> > the two calls could just move to arch/ia64/sn/kernel/irq.c
>
> ok
>
> More items:
>
> - sal_pcibr_rrb_alloc should be merged into its only caller
I'll move the sal_pcibr_rrb_alloc() code into snia_pcibr_rrb_alloc()
> - io_init.c still has KDB ifdefs
Sorry, missed this....
> - sn_pci_set_vchan should absolutely _not_ be placed in qla1280.c,
I'll fix this.
> and while you're at it there extern for snia_pcibr_rrb_alloc should
> move to a header
OK.
> - kill HUBREG_CAST instead of touching it
OK.
> - please don't put your ASSERT into asm/ headers. In general you
> should use BUG_ON
OK.
> - why do you add a pfn_t typedef that's not used anywhere
> - the patch adds include/asm-ia64/sn/xtalk/xtalk_provider.h but there's
> no more xtalk providers
>
Missed these too.
> Your headers are still a complete mess. There's no point exposing tons
> of defails about the pci interface in include/asm - just keep and
> pci_extensions.h there for non-standard PCI APIs, and the rest should
> stay private inisde arch/ia64/sn/pci/.
I'll work on fixing this up.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Latest Altix I/O code reorganization code
2004-09-07 22:10 ` Patrick Gefre
@ 2004-09-07 22:16 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2004-09-07 22:16 UTC (permalink / raw)
To: Patrick Gefre; +Cc: Christoph Hellwig, linux-ia64, linux-kernel
On Tue, Sep 07, 2004 at 05:10:25PM -0500, Patrick Gefre wrote:
> > of interface beteen upper pci dma code and pcibr code ignored)
> >
>
> Guess I'm confused about this then. Are you suggesting putting the pcibr_dma files
> into the pci_dma.c code and not having a pcibr_dma interface ?? The api is there because
> pcibr is an ASIC and is ASIC specific.
No, absolutely not. I suggested you remove the remaining ASIC-specific
bits from pci_dma.c, namely the decision when to use direct translation
and where to use ates, and the flags and only have a single mapping
routine calling into pcibr code with a prototype ala:
dma_addr_t picbr_dma_map(struct pci_dev *dev, unsigned long phys, size_t size);
> > > pci_extension.c:
> > >
> > > - dito. Why does this single function need a separate file?
> >
> > Not addressed. In general your file organization is mess still.
> >
>
> How is this:
> arch/ia64/sn/pci/
> pci_dma.c
> pci_extension.c
> pcibr/
> pcibr_ate.c
> pcibr_dma.c
> pcibr_provider.c
> pcibr_reg.c
>
> arch/ia64/sn/kernel/
> bte_error.c
> io_init.c
> iomv.c
>
> Since pcibr is an ASIC it makes sense for it to have its own directory. There are
> other ASIC interfaces that will be put in in the not too distant future and they
> will go in separate directories also.
Isn't the pcibr_ prefix enough? This isn't something that would hold up
merging, but I think the additional level is a little silly.
> I will inline free_ate_resource(), alloc_ate_resource() and ate_write(). I want to
> keep the ate code in a separate file - since it is a group of functions with a similar
> theme and is non-trivial.
Okay..
> > > of struct tiocp and pic_s (and kill the _s postifx) in syruct pcibus_info.
> > > the volatiles looks bogus, if you need it you're missing memorry barries.
> >
> > the type pointer isn't done. Any specific reason?
> >
>
> I'm confused on this too. The struct defs are for different MMR sets - so we do need
> to use different types of pointers.
What about adding an union of both _typed_ pointers so you don't need
to cast everytime?
Anyway, it looks like we're making nice progress, thanks for the work.
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2004-09-07 22:17 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-08-04 20:14 Altix I/O code reorganization Pat Gefre
2004-08-05 0:31 ` Grant Grundler
2004-08-05 18:16 ` Greg KH
2004-08-05 20:51 ` Pat Gefre
2004-08-05 21:08 ` Greg KH
2004-08-05 21:32 ` Jesse Barnes
2004-08-05 21:36 ` Greg KH
2004-08-06 13:18 ` Christoph Hellwig
2004-08-06 13:51 ` Keith Owens
2004-08-06 13:55 ` Christoph Hellwig
2004-08-06 15:47 ` Russ Anderson
2004-08-06 16:19 ` Jesse Barnes
2004-08-07 10:58 ` Christoph Hellwig
2004-08-11 23:24 ` Patrick Gefre
2004-08-12 9:15 ` Christoph Hellwig
2004-08-12 14:47 ` Jesse Barnes
2004-08-12 15:21 ` Christoph Hellwig
2004-08-27 15:10 ` Latest Altix I/O code reorganization code Patrick Gefre
2004-08-27 15:14 ` Patrick Gefre
2004-08-27 15:21 ` Christoph Hellwig
2004-08-27 15:35 ` Patrick Gefre
2004-08-27 15:44 ` Christoph Hellwig
2004-09-03 23:12 ` Latest Altix I/O code rewrite Patrick Gefre
2004-08-27 15:36 ` Latest Altix I/O code reorganization code Christoph Hellwig
2004-08-27 15:45 ` Christoph Hellwig
2004-08-27 16:32 ` Patrick Gefre
2004-08-27 15:54 ` Christoph Hellwig
2004-08-27 16:06 ` Patrick Gefre
2004-08-27 16:21 ` Christoph Hellwig
2004-08-29 6:39 ` Keith Owens
2004-08-29 7:16 ` Sam Ravnborg
2004-08-29 7:22 ` Keith Owens
2004-09-03 23:40 ` Christoph Hellwig
2004-09-07 22:10 ` Patrick Gefre
2004-09-07 22:16 ` Christoph Hellwig
2004-08-27 17:15 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).