Kevin, Can you please reply to my comments below?
Thanks, Hemant > -----Original Message----- > From: > davinci-linux-open-source-bounces+hemantp=ti....@linux.davinci > dsp.com > [mailto:davinci-linux-open-source-bounces+hemantp=ti....@linux > .davincidsp.com] On Behalf Of Pedanekar, Hemant > Sent: Saturday, May 23, 2009 9:23 AM > To: Kevin Hilman > Cc: davinci-linux-open-source@linux.davincidsp.com > Subject: RE: [PATCH 1/2] ARM: DaVinci: DM646x PCIHOST: Core > files modifications for PCI PINMUX and I/O > > > > > -----Original Message----- > > From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > > Sent: Friday, May 22, 2009 4:48 AM > > To: Pedanekar, Hemant > > Cc: davinci-linux-open-source@linux.davincidsp.com > > Subject: Re: [PATCH 1/2] ARM: DaVinci: DM646x PCIHOST: Core files > > modifications for PCI PINMUX and I/O > > > > Hemant Pedanekar <hema...@ti.com> writes: > > > > > The changes mainly target two areas: 1) Provide clock structure, > > > setting > > up PCI > > > PINMUX and perform board specific fix-ups for PCI Host > core to begin > > > configuration/enumeraion and 2) Provide PCI IO access through > > > in/outx() > > and > > > ioread/iowritex(). > > > > > > The arm-kernel configuration is updated to support PCI Bus for > > > DM646x > > builds. > > > > > > In addition, I/O mapping macros are moved inside > hardware.h to avoid > > > the > > need > > > for inclusion of io.h in assembly files which results in assembler > > errors for > > > PCI I/O functions, as well as serial.h, which results in > unresolved > > symbols for > > > PCI I/O functions (in uncompress code). > > > > > > Signed-off-by: Hemant Pedanekar <hema...@ti.com> > > > --- > > > arch/arm/Kconfig | 2 +- > > > arch/arm/mach-davinci/Kconfig | 1 + > > > arch/arm/mach-davinci/Makefile | 6 + > > > arch/arm/mach-davinci/board-dm646x-evm.c | 82 ++++++++ > > > arch/arm/mach-davinci/dm646x.c | 22 ++- > > > arch/arm/mach-davinci/include/mach/dm646x.h | 33 +++- > > > arch/arm/mach-davinci/include/mach/entry-macro.S | 2 +- > > > arch/arm/mach-davinci/include/mach/hardware.h | 21 ++ > > > arch/arm/mach-davinci/include/mach/io.h | 237 > > ++++++++++++++++++++-- > > > arch/arm/mach-davinci/include/mach/mux.h | 10 +- > > > arch/arm/mach-davinci/include/mach/serial.h | 2 +- > > > arch/arm/mach-davinci/include/mach/vmalloc.h | 2 +- > > > 12 files changed, 390 insertions(+), 30 deletions(-) > > > > > > > Just some general comments without digging too much into the PCI > > specific parts... > > > > You're doing lots of things in this patch, it should be > split break it > > up into smaller pieces with better descriptions. > > > > You could break out some cleanup/fixup patches, and the clock/mux > > defines into separate patches, the PCI accessor functions etc. etc. > > > That seems better approach. I will do that. > > > The header rework should be a separate patch with a clearer > > description of the problem. If the problem is with incluing from > > assembly files, then there's probably a better way to do this like > > using #ifdef __ASSEMBLER__. > > > Its not just assembler errors, I also get undefined > references errors from uncompress code as io.h gets included > there (through serial.h, I guess) and the PIC I/O functions > are not built. > > Another approach to get around these errors was to add > EXTRA_FLAGS in arch/arm/boot/compressed/Makefile and then use > that flag along with __ASSEMBLER__ to skip PCI I/O functions in io.h. > > I also observed that most of the other arm/mach-xxx had > IO_ADDRESS etc. defined in hardware.h. So they would not get > such errors as io.h was not included. > > Which option do you suggest? > > > > Also, is PCI going to be specific to the dm646x forever? Something > > tells me no. Some of the PCI stuff you put into dm646x.h should > > probably be in a more generic header like <mach/pci.h> or something. > > > Are you referring to "pcibios_assign_all_busses"? If so, you > are correct that it is not really DM6467 specific. But the > other #defines (such as addresses) are still DM6467 specific > aren't they? > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index > > > bf935b0..3324c02 100644 > > > --- a/arch/arm/Kconfig > > > +++ b/arch/arm/Kconfig > > > @@ -776,7 +776,7 @@ config ISA_DMA_API > > > bool > > > > > > config PCI > > > - bool "PCI support" if ARCH_INTEGRATOR_AP || ARCH_VERSATILE_PB || > > ARCH_IXP4XX || ARCH_KS8695 || MACH_ARMCORE > > > + bool "PCI support" if ARCH_INTEGRATOR_AP || ARCH_VERSATILE_PB || > > ARCH_IXP4XX || ARCH_KS8695 || MACH_ARMCORE || ARCH_DAVINCI_DM646x > > > help > > > Find out whether you have a PCI motherboard. PCI is > the name of a > > > bus system, i.e. the way the CPU talks to the other > stuff inside > > > diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach- > > davinci/Kconfig > > > index c460615..2ce0ffd 100644 > > > --- a/arch/arm/mach-davinci/Kconfig > > > +++ b/arch/arm/mach-davinci/Kconfig > > > @@ -17,6 +17,7 @@ config ARCH_DAVINCI_DM644x config > > > ARCH_DAVINCI_DM646x > > > bool "DaVinci 646x based system" > > > select AINTC > > > + select DAVINCI_MUX if PCI > > > > > > config ARCH_DAVINCI_DM355 > > > bool "DaVinci 355 based system" > > > diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach- > > davinci/Makefile > > > old mode 100644 > > > new mode 100755 > > > index 6da7b63..7323c83 > > > --- a/arch/arm/mach-davinci/Makefile > > > +++ b/arch/arm/mach-davinci/Makefile > > > @@ -9,12 +9,18 @@ obj-y := time.o > clock.o serial.o io.o > > psc.o \ > > > sram.o > > > > > > obj-$(CONFIG_DAVINCI_MUX) += mux.o > > > +obj-$(CONFIG_PCI) += pci-generic.o > > > > You add files to the Makefile which are not added in this patch. > > This will break bisecting. > > > > > # Chip specific > > > obj-$(CONFIG_ARCH_DAVINCI_DM644x) += dm644x.o > > > obj-$(CONFIG_ARCH_DAVINCI_DM646x) += dm646x.o > > > obj-$(CONFIG_ARCH_DAVINCI_DM355) += dm355.o > > > > > > +# PCI - Chip specific > > > +ifeq ($(CONFIG_PCI), y) > > > +obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pci-dm6467.o > > > +endif > > > + > > > > Ditto. > > > I will move Kconfig and Makefile patches to PCI driver patch set. > > > [...] > > > > Kevin > > > > Thanks for your comments! > Hemant > > > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source > _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
