Hi Kevin, See inline.
Thanks Sneha > -----Original Message----- > From: Kevin Hilman [mailto:[EMAIL PROTECTED] > Sent: Friday, November 07, 2008 9:00 PM > To: Narnakaje, Snehaprabha > Cc: Rajashekhara, Sudhakar; Paulraj, Sandeep; Subrahmanya, Chaithrika; > davinci-linux-open-source@linux.davincidsp.com > Subject: Re: staging tree for dm6467 and dm355 support > > "Narnakaje, Snehaprabha" <[EMAIL PROTECTED]> writes: > > > Kevin, > > > > Looks like you have made good progress on getting the GIT more > > active. Thanks a lot for pushing the patches into the staging area. > > > > Regarding the base address definitions across DaVinci series of > >devices, I am trying to summarize the changes and raise concern for > >some of them: > > Thank you for reviewing these changes. > > To summarize my overarcing goal here: the main purpose is to move all > the board/chip specific settings (like base addresses) into board/chip > specific code. Drivers should NEVER be using base addresses directly > (like davinci_nand currently does.) Rather, board/chip code should be > filling out platform_data structs and passing them into the driver. > This allows the same driver to work across multiple platforms. I agree with you on not having/using base addresses directly in the driver. I have a related question though. Currently we have some of the platform_data structures defined in board-evm file (e.g. NAND, NOR and ATA) and the rest in devices.c. Should all be defined in devices.c? > > > 1. Currently AEMIF Data CE base addresses are defined in the > > arch/arm/mach-davinci/include/mach/hardware.h file. These addresses > > are the same for DM644x, DM355 and DM365. However they are different > > for DM6467. How do we then share and use these base addresses across > > DaVinci devices? > > OK, this is what I was worried about. I wasn't sure about those and > that's why were left in hardware.h. I now have updated docs for > 6446, 6447 and 355, but not yet for 365 so I will go back over those. > > Basically, the more devices there are, the less likely that any of the > base addresses will be common. > > In the end, I think hardware.h may be very sparse, and most things > defined in <device>.h. > > What remains is whether to come up with a strategy of how to name > things that are common between some devices but not others, or just > duplicate them them across each <device>.h with a different <DEVICE>_ > prefix. It makes a little more init code because there will more 'if > cpu_is_foo()' statements, but that doesn't bother me much. In fact, > it will probably be more readable. > > > 2. PLL base address is defined and used in > > arch/arm/mach-davinci/clock.c > > > > 3. DMA base address is defined and used in arch/arm/mach-davinci/dma.c > > > > 4. GPIO base address is defined in > > arch/arm/mach-davinci/include/mach/gpio.h Why do I see the same > > header file in include/asm-arm/arch/davinci/ (others like serial.h > > and system.h are also in both the locations) > > Hmm, include/asm-arm/arch-davinci is the old location and should be > empty. I will delete those. This is a remnant of the 2.6.27 merge > which look like I didn't quite finish cleaning up. > > > 5. System module and ATA base addresses are defined in > > arch/arm/mach-davinci/include/mach/hardware.h. This should be OK, as > > System module base address is the same across all DaVinci > > devices. ATA base address is also common across DM644x and DM646x. > > OK, than ATA base should be moved from hardware.h into each > <device>.h and the init code updated. > > > 6. INTC base address in arch/arm/mach-davinci/include/mach/irqs.h; > > DDR and IRAM base addresses in > > arch/arm/mach-davinci/include/mach/memory.h; PSC base address in > > arch/arm/mach-davinci/psc.c; Timer base addresses in > > arch/arm/mach-davinci/time.c and USB base address in > > arch/arm/mach-davinci/usb.c. This should OK, as they are same across > > all devices. > > > 7. UART base addresses for DaVinci are defined in > > arch/arm/mach-davinci/include/mach/serial.h. But we also have a > > DM355 specific definition (UART2) in > > arch/arm/mach-davinci/serial.c. Should we be generalizing this and > > keeping them in one single header file? > > I originally wanted to move them all into serial.c, but the early boot > and uncompress code uses UART0 and so needs that header. > > I think these can stay in serial.h for now, but the DM355 one > should probably be moved from serial.c into dm355.h. > > > 8. McBSP base addresses for DaVinci are defined > > arch/arm/mach-davinci/include/mach/mcbsp.h. These definitions are > > different for these EVMs. So should we be defining mcbsp_dm644x.h > > mcbsp_dm355.h and so on? > > Actually, these should just go into <device>.h instead of mcbsp_<device>.h > > The current location of this to mcbsp.h was just until someone fixes > the ASoC driver. There is lots of board/chip specific code in > sound/soc/davinci and it should be moved into board-*.c files. Once > that is done, then the board-* files will include <device>.h files, > fill out the platform_data and pass it into ASoC. > > > 9. All other peripheral (or device driver) specific base addresses > > are defined in arch/arm/mach-davinci/devices.c. Some of the base > > addresses are same across DaVinci devices, but some are different > > (e.g. EMAC base address is same for DM644x and DM646x, but different > > for DM365). It is true for other peripherals such as MMC/SD and SPI. > > OK, then the thing to do here is move these defines into <device>.h > files. Then devices.c will have to include each <device>.h and then > use cpu_is_* at runtime and figure out which base address to use. > > Thanks again for your review, > > I'll work more on this rework next week. Let us know, if there is anything we can do to help you out. > > Kevin > > > >> -----Original Message----- > >> From: davinci-linux-open-source- > >> [EMAIL PROTECTED] [mailto:davinci-linux- > >> [EMAIL PROTECTED] On Behalf > Of > >> Kevin Hilman > >> Sent: Thursday, November 06, 2008 9:06 PM > >> To: Rajashekhara, Sudhakar; Paulraj, Sandeep; Subrahmanya, Chaithrika > >> Cc: davinci-linux-open-source@linux.davincidsp.com > >> Subject: staging tree for dm6467 and dm355 support > >> > >> Hello, > >> > >> While I review the dm6467 and dm355 patches, and prepare for an update > >> to newer kernels, I've created a temporary staging branch[1] where > >> I've applied the dm646x and dm355 patches. > >> > >> On top of that, I've added a small series of rework patches where I've > >> reworked how and where base addresses are defined, init functions are > >> declared. I'm very interested in your comments on this layout. > >> > >> Basically, what I've done is got rid of most of the base address > >> definitions as global defines. The remaining ones that need to global > >> and that are common to ALL chips in the family will live in > >> hardware.h. Ones that need to be global and are chip specific should > >> live in <chipname>.h > >> > >> Note that I said "need to be global". Most base address defines do > >> not need to be in a global header. They are only ever used in > >> chip/board specific init code to fill in platform_data which is then > >> passed to the driver. Drivers should _never_ be using base address > >> defines directly. > >> > >> I've compile and boot tested this kernel on dm6446, dm6467 and dm355. > >> > >> Kevin > >> > >> [1] See the 'tmp/staging branch in DaVinci git. > >> > >> http://source.mvista.com/git/?p=linux-davinci- > >> 2.6.git;a=shortlog;h=refs/heads/tmp/ti-staging > >> > >> _______________________________________________ > >> 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
