Altix I/O code reorganization

Altix I/O code reorganization

[Pat Gefre]

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

Re: Altix I/O code reorganization

[Grant Grundler]

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

Re: Altix I/O code reorganization

[Greg KH]

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

Re: Altix I/O code reorganization

[Pat Gefre]

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

Re: Altix I/O code reorganization

[Greg KH]

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

Re: Altix I/O code reorganization

[Jesse Barnes]

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

Re: Altix I/O code reorganization

[Greg KH]

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

Re: Altix I/O code reorganization

[Christoph Hellwig]

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

Re: Altix I/O code reorganization

[Jesse Barnes]

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

Re: Altix I/O code reorganization

[Christoph Hellwig]

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.

Re: Altix I/O code reorganization

[Patrick Gefre]

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.

Re: Altix I/O code reorganization

[Christoph Hellwig]

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. 

Re: Altix I/O code reorganization

[Jesse Barnes]

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

Re: Altix I/O code reorganization

[Christoph Hellwig]

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.

Latest Altix I/O code reorganization code

[Patrick Gefre]

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

Re: Latest Altix I/O code reorganization code

[Patrick Gefre]

Patch 1 of 26

This patch is too large for email see:

ftp://oss.sgi.com/projects/sn2/sn2-update/001-kill-files

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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)

Re: Latest Altix I/O code reorganization code

[Patrick Gefre]

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.

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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.

Re: Latest Altix I/O code reorganization code

[Pat Gefre]

Patch 2 of 26


# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/08/25 10:54:51-05:00 pfg@sgi.com 
#   kill ioconfig_bus
# 
# BitKeeper/deleted/.del-ioconfig_bus.c~6cf1ae3dd328053d
#   2004/08/25 10:54:11-05:00 pfg@sgi.com +0 -0
#   Delete: arch/ia64/sn/io/drivers/ioconfig_bus.c
# 
diff -Nru a/arch/ia64/sn/io/drivers/ioconfig_bus.c b/arch/ia64/sn/io/drivers/ioconfig_bus.c
--- a/arch/ia64/sn/io/drivers/ioconfig_bus.c	2004-08-25 11:01:36 -05:00
+++ /dev/null	Wed Dec 31 16:00:00 196900
@@ -1,382 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
- *
- *  ioconfig_bus - SGI's Persistent PCI Bus Numbering.
- *
- * Copyright (C) 1992-1997, 2000-2003 Silicon Graphics, Inc.  All rights reserved.
- */
-
-#include <linux/types.h>
-#include <linux/slab.h>
-#include <linux/ctype.h>
-#include <linux/module.h>
-#include <linux/init.h>
-
-#include <linux/pci.h>
-
-#include <asm/uaccess.h>
-#include <asm/sn/sgi.h>
-#include <asm/io.h>
-#include <asm/sn/iograph.h>
-#include <asm/sn/hcl.h>
-#include <asm/sn/labelcl.h>
-#include <asm/sn/sn_sal.h>
-#include <asm/sn/addrs.h>
-#include <asm/sn/ioconfig_bus.h>
-
-#define SGI_IOCONFIG_BUS "SGI-PERSISTENT PCI BUS NUMBERING"
-#define SGI_IOCONFIG_BUS_VERSION "1.0"
-
-/*
- * Some Global definitions.
- */
-static vertex_hdl_t ioconfig_bus_handle;
-static unsigned long ioconfig_bus_debug;
-static struct ioconfig_parm parm;
-
-#ifdef IOCONFIG_BUS_DEBUG
-#define DBG(x...)	printk(x)
-#else
-#define DBG(x...)
-#endif
-
-static u64 ioconfig_activated;
-static char ioconfig_kernopts[128];
-
-/*
- * For debugging purpose .. hardcode a table ..
- */
-struct  ascii_moduleid *ioconfig_bus_table;
-
-static int free_entry;
-static int new_entry;
-
-int next_basebus_number;
-
-void
-ioconfig_get_busnum(char *io_moduleid, int *bus_num)
-{
-	struct	ascii_moduleid  *temp;
-	int index;
-
-	DBG("ioconfig_get_busnum io_moduleid %s\n", io_moduleid);
-
-	*bus_num = -1;
-	temp = ioconfig_bus_table;
-	if (!ioconfig_bus_table)
-		return;
-	for (index = 0; index < free_entry; temp++, index++) {
-		if ( (io_moduleid[0] = temp->io_moduleid[0]) &&
-		     (io_moduleid[1] = temp->io_moduleid[1]) &&
-		     (io_moduleid[2] = temp->io_moduleid[2]) &&
-		     (io_moduleid[4] = temp->io_moduleid[4]) &&
-		     (io_moduleid[5] = temp->io_moduleid[5]) ) {
-			*bus_num = index * 0x10;
-			return;
-		}
-	}
-
-	/*
-	 * New IO Brick encountered.
-	 */
-	if (((int)io_moduleid[0]) = 0) {
-		DBG("ioconfig_get_busnum: Invalid Module Id given %s\n", io_moduleid);
-		return;
-	}
-
-	io_moduleid[3] = '#';
-	strcpy((char *)&(ioconfig_bus_table[free_entry].io_moduleid), io_moduleid);
-	*bus_num = free_entry * 0x10;
-	free_entry++;
-}
-
-static void
-dump_ioconfig_table(void)
-{
-
-	int index = 0;
-	struct ascii_moduleid *temp;
-
-	temp = ioconfig_bus_table;
-	if (!temp) {
-		DBG("ioconfig_bus_table tabel empty\n");
-		return;
-	}
-	while (index < free_entry) {
-		DBG("ASSCI Module ID %s\n", temp->io_moduleid);
-		temp++;
-		index++;
-	}
-}
-
-/*
- * nextline
- *	This routine returns the nextline in the buffer.
- */
-int nextline(char *buffer, char **next, char *line)
-{
-
-	char *temp;
-
-	if (buffer[0] = 0x0) {
-		return(0);
-	}
-
-	temp = buffer;
-	while (*temp != 0) {
-		*line = *temp;
-		if (*temp != '\n'){
-			*line = *temp;
-			temp++; line++;
-		} else
-			break;
-	}
-
-	if (*temp = 0)
-		*next = temp;
-	else
-		*next = ++temp;
-
-	return(1);
-}
-
-/*
- * build_pcibus_name
- *	This routine parses the ioconfig contents read into
- *	memory by ioconfig command in EFI and builds the
- *	persistent pci bus naming table.
- */
-int
-build_moduleid_table(char *file_contents, struct ascii_moduleid *table)
-{
-	/*
-	 * Read the whole file into memory.
-	 */
-	int rc;
-	char *name;
-	char *temp;
-	char *next;
-	char *curr;
-	char *line;
-	struct ascii_moduleid *moduleid;
-
-	line = kmalloc(256, GFP_KERNEL);
-	name = kmalloc(125, GFP_KERNEL);
-	if (!line || !name) {
-		if (line)
-			kfree(line);
-		if (name)
-			kfree(name);
-		printk("build_moduleid_table(): Unabled to allocate memmory");
-		return -ENOMEM;
-	}
-
-	memset(line, 0,256);
-	memset(name, 0, 125);
-	moduleid = table;
-	curr = file_contents;
-	while (nextline(curr, &next, line)){
-
-		DBG("curr 0x%lx next 0x%lx\n", curr, next);
-
-		temp = line;
-		/*
-		 * Skip all leading Blank lines ..
-		 */
-		while (isspace(*temp))
-			if (*temp != '\n')
-				temp++;
-			else
-				break;
-
-		if (*temp = '\n') {
-			curr = next;
-			memset(line, 0, 256);
-			continue;
-		}
- 
-		/*
-		 * Skip comment lines
-		 */
-		if (*temp = '#') {
-			curr = next;
-			memset(line, 0, 256);
-			continue;
-		}
-
-		/*
-		 * Get the next free entry in the table.
-		 */
-		rc = sscanf(temp, "%s", name);
-		strcpy(&moduleid->io_moduleid[0], name);
-		DBG("Found %s\n", name);
-		moduleid++;
-		free_entry++;
-		curr = next;
-		memset(line, 0, 256);
-	}
-
-	new_entry = free_entry;
-	kfree(line);
-	kfree(name);
-
-	return 0;
-}
-
-int
-ioconfig_bus_init(void)
-{
-
-	DBG("ioconfig_bus_init called.\n");
-
-	ioconfig_bus_table = kmalloc( 512, GFP_KERNEL );
-	if (!ioconfig_bus_table) {
-		printk("ioconfig_bus_init : cannot allocate memory\n");
-		return -1;
-	}
-
-	memset(ioconfig_bus_table, 0, 512);
-
-	/*
-	 * If ioconfig options are given on the bootline .. take it.
-	 */
-	if (*ioconfig_kernopts != '\0') {
-		/*
-		 * ioconfig="..." kernel options given.
-		 */
-		DBG("ioconfig_bus_init: Kernel Options given.\n");
-		if ( build_moduleid_table((char *)ioconfig_kernopts, ioconfig_bus_table) < 0 )
-			return -1;
-		(void) dump_ioconfig_table();
-	}
-	return 0;
-}
-
-void
-ioconfig_bus_new_entries(void)
-{
-	int index;
-	struct ascii_moduleid *temp;
-
-	if ((ioconfig_activated) && (free_entry > new_entry)) {
-		printk("### Please add the following new IO Bricks Module ID \n");
-		printk("### to your Persistent Bus Numbering Config File\n");
-	} else
-		return;
-
-	index = new_entry;
-	if (!ioconfig_bus_table) {
-		printk("ioconfig_bus_table table is empty\n");
-		return;
-	}
-	temp = &ioconfig_bus_table[index];
-        while (index < free_entry) {
-                printk("%s\n", (char *)temp);
-		temp++;
-                index++;
-        }
-	printk("### End\n");
-
-}
-static int ioconfig_bus_ioctl(struct inode * inode, struct file * file,
-        unsigned int cmd, unsigned long arg)
-{
-	/*
-	 * Copy in the parameters.
-	 */
-	if (copy_from_user(&parm, (char *)arg, sizeof(struct ioconfig_parm)))
-		return -EFAULT;
-	parm.number = free_entry - new_entry;
-	parm.ioconfig_activated = ioconfig_activated;
-	if (copy_to_user((char *)arg, &parm, sizeof(struct ioconfig_parm)))
-		return -EFAULT;
-
-	if (!ioconfig_bus_table)
-		return -EFAULT;
-
-	if (copy_to_user((char *)parm.buffer, &ioconfig_bus_table[new_entry], sizeof(struct  ascii_moduleid) * (free_entry - new_entry)))
-		return -EFAULT;
-
-	return 0;
-}
-
-/*
- * ioconfig_bus_open - Opens the special device node "/dev/hw/.ioconfig_bus".
- */
-static int ioconfig_bus_open(struct inode * inode, struct file * filp)
-{
-	if (ioconfig_bus_debug) {
-        	DBG("ioconfig_bus_open called.\n");
-	}
-
-        return(0);
-
-}
-
-/*
- * ioconfig_bus_close - Closes the special device node "/dev/hw/.ioconfig_bus".
- */
-static int ioconfig_bus_close(struct inode * inode, struct file * filp)
-{
-
-	if (ioconfig_bus_debug) {
-        	DBG("ioconfig_bus_close called.\n");
-	}
-
-        return(0);
-}
-
-struct file_operations ioconfig_bus_fops = {
-	.ioctl	= ioconfig_bus_ioctl,
-	.open	= ioconfig_bus_open,	/* open */
-	.release=ioconfig_bus_close	/* release */
-};
-
-
-/*
- * init_ifconfig_bus() - Boot time initialization.  Ensure that it is called 
- *	after hwgfs has been initialized.
- *
- */
-int init_ioconfig_bus(void)
-{
-	ioconfig_bus_handle = hwgraph_register(hwgraph_root, ".ioconfig_bus",
-		        0, 0,
-			0, 0,
-			S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP, 0, 0,
-			&ioconfig_bus_fops, NULL);
-
-	if (ioconfig_bus_handle = NULL) {
-		panic("Unable to create SGI PERSISTENT BUS NUMBERING Driver.\n");
-	}
-
-	return 0;
-}
-
-static int __init ioconfig_bus_setup (char *str)
-{
-
-	char *temp;
-
-	DBG("ioconfig_bus_setup: Kernel Options %s\n", str);
-
-	temp = (char *)ioconfig_kernopts;
-	memset(temp, 0, 128);
-	while ( (*str != '\0') && !isspace (*str) ) {
-		if (*str = ',') {
-			*temp = '\n';
-			temp++;
-			str++;
-			continue;
-		}
-		*temp = *str;
-		temp++;
-		str++;
-	}
-
-	return(0);
-		
-}
-__setup("ioconfig=", ioconfig_bus_setup);

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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.

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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

Re: Latest Altix I/O code reorganization code

[Patrick Gefre]

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

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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/

Re: Latest Altix I/O code reorganization code

[Patrick Gefre]

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.

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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/.

Re: Latest Altix I/O code reorganization code

[Patrick Gefre]

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.

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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.

Re: Latest Altix I/O code reorganization code

[Christoph Hellwig]

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..

Re: Latest Altix I/O code reorganization code

[Keith Owens]

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().

Re: Latest Altix I/O code reorganization code

[Sam Ravnborg]

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

Re: Latest Altix I/O code reorganization code

[Keith Owens]

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.

Re: Altix I/O code reorganization

[Keith Owens]

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.

Re: Altix I/O code reorganization

[Christoph Hellwig]

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.

Re: Altix I/O code reorganization

[Russ Anderson]

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