Kevin, Thanks for the review. Please see my comments below.
> -----Original Message----- > From: Kevin Hilman [mailto:[email protected]] > Sent: Friday, October 16, 2009 4:17 AM > To: Pedanekar, Hemant > Cc: [email protected] > Subject: Re: [PATCH v2 2/4] davinci: dm646x: Add PCI host controller > driver > > Hemant Pedanekar <[email protected]> writes: > > > The driver code is organized in two parts, DM646x specific part and > 'generic' > > part which could be easily adopted on other platform/SoCs having same > PCI core > > as in DM6467. > > First of all, sorry for the *huge* delay in reviewing this series. > It's been > 2 months since this original posting. Feel free to harass > me more often if I let things slip more than a couple weeks. > > > Also overrides io accessor functions to support PCI IO. > > > > Currently PCI interrupt mapping (INTA to D) is not supported as the EVM > has no > > PCI interrupt pins connected to interript controller. > > A link to the PCI doc (spruer2) would be helpful here too. > Will add. > > Signed-off-by: Hemant Pedanekar <[email protected]> > > Overall, this looks pretty good. After some general comments, this > should probably be posted to linux-arm-kernel for some broader review > by folks that may have some more PCI background. After that, I'll > pull into davinci git. > > Some general comments: > > - Documentation should follow kerneldoc style > - see preferred multi-line comment sytle in Documentation/CodingStyle Ok. > - do you want to enable DEBUG for the pci-* files when CONFIG_PCI_DEBUG=y? > I thought of keeping it separate as there will be lot more debugging info printed with DEBUG from the driver than kernel PCI module. But as you suggest, tying with CONFIG_PCI_DEBUG looks cleaner than having to edit files. > > --- > > arch/arm/mach-davinci/include/mach/dm646x.h | 5 + > > arch/arm/mach-davinci/include/mach/io.h | 222 ++++++++++++ > > arch/arm/mach-davinci/include/mach/pci.h | 44 +++ > > arch/arm/mach-davinci/pci-dm646x.c | 139 ++++++++ > > arch/arm/mach-davinci/pci-generic.c | 493 > +++++++++++++++++++++++++++ > > not a big deal, but I think pci-common.c is probably a better name. > I will change it. [...] > > + > > +#define IS_PCI_IO(p) (((p) >= PCIBIOS_MIN_IO) \ > > + && ((p) <= PCIBIOS_MAX_IO)) > > This is just a minor optimization, but I wonder if this would be a > little faster without having to do two compares for every access. > > Since "normal" users of the accessors will all be using virtual > addresses and since the PCI addresses will always be physical, the > access functions could be slightly faster if they just did a raw > access for any virtual address and a PCI access otherwise. Something > like: > > if (addr >= VMALLOC_START) > __raw_<access> > return; > > /* then do PCI function here */ > I think this is better. I will add that 'if' check for normal accesses prior going into PCI i/o. But do we still need to guard PCI i/o access with 'IS_PCI_IO' just to avoid any outside I/O access over PCI - though PCI will any way abort such access? what do you think? [...] > > +static inline u8 _davinci_inb(u32 addr) > > +{ > > + if (IS_PCI_IO(addr)) { > > + u32 value; > > + ti_pci_io_read(addr, 1, &value); > > + return (u8)value; > > + } else { > > + return __raw_readb(__typesafe_io(addr)); > > + } > > extra braces not needed here, or several places below > I will change this. [...] > > +static inline void _davinci_insl(u32 addr, void *data, u32 llen) > > +{ > > + while (llen--) > > + *(u32 *)data++ = _davinci_inl((u32)((u32 *)addr++)); > > +} > > + > > +static inline unsigned int _davinci_ioread8(void __iomem *port) > > +{ > > + return (unsigned int)_davinci_inb((unsigned long __force)port); > > +} > > + > > +static inline void _davinci_ioread8_rep(void __iomem *port, void *dst, > > + unsigned long count) > > +{ > > + _davinci_insb((unsigned long __force)port, dst, count); > > +} > > + [...] > > A minor readability issue, but instead of blen, wlen and llen, can you > just use count for all of them. > Yes, that will look better. [...] > > +#undef DEBUG > > + > > +#include <linux/kernel.h> /* pr_ wrappers */ > > +#include <linux/pci.h> /* PCI data structures */ > > +#include <asm/mach/pci.h> /* hw_pci */ > > > > +#include <linux/types.h> > > + > > +#include "pci.h" /* Platform spec */ > > + > > +#include <mach/cputype.h> > > +#include <mach/dm646x.h> /* SoC reg defs, __REG, IO info etc */ > > +#include <linux/clk.h> /* For clk_enable interface */ > > The extra comments after the #includes aren't needed there and tend to > get stale. Just drop them. > Ok. > > + > > +/* NOTE: > > + * Most of the code in this file assumes that it is runnig as PCI Host > and > > + * configures the system accordingly. The only place where it is > checked if we > > + * are booted through PCI Boot, is the function dm646x_pci_init (below) > before > > + * it registers with the bios. Thus, if we were not hosting the PCI bus > the > > + * other functions wouldn;t get called anyway as the pci bios will not > be > > + * invoked. > > + */ > > FYI... I tried this series on top of current davinci git, enabled > CONFIG_PCI and tested on my standalone dm646x EVM. The kernel reboots > immediately on startup. Is there a way to check for neither PCI host > or target and abort the init phase? > Did you have ATA enabled along with PCI? I think CPLD setting for ATA (in cpld_reg0_probe) conflicts in this case. I will add !HAS_PCI check there to skip setting CPLD (just like dm646x_init_ide() is skipped) when PCI is enabled. > > +/* Forward declarations */ > > +static void __init dm646x_pci_preinit(void); > > +static int __init dm646x_pci_map_irq(struct pci_dev *dev, u8 slot, u8 > pin); > > + > > +/* Specify the physical address and size information for Host windows > to be > > + * configured in BAR0-5 registers. Note that _ALL_ of the 6 entries > must be > > + * filled. For entries with no real window to be mapped, set > bar_window_size=0. > > + */ > > +struct pcihost_bar_cfg bar_cfg[6] = { > > + { PHYS_OFFSET /* bar_window_phys */ , 0x8000000 /* bar_window_size > */}, > > + { 0 , 0 > }, > > + { 0 , 0 > }, > > + { 0 , 0 > }, > > + { 0 , 0 > }, > > + { 0 , 0 > } > > +}; > > Hmm, do you really want to expose the entire memory map to the host by > default? Seems like something that a board file should be allowed to > configure and override. > Actually I was not sure about where to handle some board/config specific stuff (such as this and pci_map_irq, bootmode checks etc) so I thought of using this file and keeping pci-generic.c independent of it. Do you have some other ideas? [...] > > +int dm646x_pci_map_irq(struct pci_dev *dev, u8 slot, u8 pin) > > +{ > > + /* > > + * Standard EVM doesn't have any dedicated lines connected to PCI > > + * Interrupts. > > + */ > > + > > + return -1; > > +} > > + > > +/* dm646x_pci_init > > + * PCI system initialization. Fills in required information for PCI > BIOS to > > + * perrform enumeratios and invokes pci_common_init. > > + */ > > +static int __init dm646x_pci_init(void) > > +{ > > + if (cpu_is_davinci_dm646x()) { > > Is this cpu_is* necessary? we're in dm646x specific code already, right? > Wouldn't this be required in case of davinci_all_defconfig as dm646x_pci_init is subsys_initcall? [...] > > + /* Overwrite the resources which were set up by default by > > + * pcibios_init_hw > > + */ > > + res = kzalloc(sizeof(*res) * 2, GFP_KERNEL); > > + if (res == NULL) { > > + panic("PCI: resource structure allocation failed.\n"); > > + /* Not reached */ > > + return 0; > > + } > > You shouldn't halt the whole kernel here on an alloc failure. You > should use a WARN instead of panic and fail gracefully. > Will change this. > [...] > > +static inline int pci_cfgio_read(u32 addr, int size, u32 *value, > > + int be, int access_type) > > +{ > > + unsigned long flags; > > + int ret_val = 0; > > + > > + spin_lock_irqsave(&ti_pci_lock, flags); > > Can taking the lock come after the check for READY? > Yes, that looks cleaner. > > + /* Check of READY. Continue even if not READY as we will catch the > error > > + * after operation. Same logic is applied for 'write' operation > below... > > + */ > > + if (!PCI_WAIT_FOR_RDY()) > > + pr_warning("PCI: CFG/IO not ready." > > + "Might need tuning CFG_PCI_READY_WAITCNT value.\n"); > > + > > + /* Clear aborts */ > > + __raw_writel((__raw_readl(PCI_REGV(PCICSRMIR)) | (0xFF << 24)), > > + PCI_REGV(PCICSRMIR)); > > + > > + __raw_writel(addr, PCI_REGV(PCIMCFGADR)); > > + > > + __raw_writel((BIT(31) | ((access_type == PCI_ACCESS_TYPE_CFG) > > + ? CFG_PCIM_CMD_CONFIG : CFG_PCIM_CMD_IO) > > + | CFG_PCIM_CMD_READ > > + | (be << 4)), PCI_REGV(PCIMCFGCMD)); > > + > > + /* Check for READY. */ > > + ret_val = !PCI_WAIT_FOR_RDY(); > > + > > + if (pci_err()) { > > + pr_debug("PCI: CFG/IO Read @%#x Failed.\n", addr); > > + *value = ~0; > > + /* Clear aborts */ > > + __raw_writel((__raw_readl(PCI_REGV(PCICSRMIR)) | (0xFF << > 24)), > > + PCI_REGV(PCICSRMIR)); > > + ret_val = -1; > > + } else { > > + *value = __raw_readl(PCI_REGV(PCIMCFGDAT)); > > + pr_debug("PCI: Config/IO read done, value = %x\n", *value); > > + } > > + > > + spin_unlock_irqrestore(&ti_pci_lock, flags); > > + return ret_val; > > +} > > + [...] > > +#define CFG_PCIM_WINDOW_SZ (0x00800000) /* Master window size */ > > Use SZ_8M. > Ok. > > +#define CFG_PCIM_WINDOW_CNT (32) /* Number of > windows */ > > +#define CFG_PCIM_MEM_START (PCIBIOS_MIN_MEM) /* PCI master memory map > > + base NOTE: We are using > > + 1:1 mapping, i.e., > > + 0x30000000 from DMSoC > > + side is translated as > > + 0x30000000 on PCI Bus */ > > +#define CFG_PCIM_MEM_END (CFG_PCIM_MEM_START \ > > + + (CFG_PCIM_WINDOW_SZ \ > > + * CFG_PCIM_WINDOW_CNT) \ > > + - 1) /* PCI master memory map size */ > > Since the WINDOW_CNT could be changed, for extra sanity, you should > probably check that this never goes past 256M. Something like this > should work (untested): > > #if CFG_PCIM_MEM_END >= SZ_256M > #error PCI memory size must be <= 256M > #endif > Do you think this check is really required? Actually CFG_PCIM_MEM_END value is used as is to do request_resource. I think (or at least my intention was that) the code in pci-generic.c (pci-common.c!) should be able to handle different CFG_PCIM_WINDOW_CNT/CFG_PCIM_WINDOW_SZ (and/or CFG_PCIM_MEM_END) values, provided 'CFG_PCIM_WINDOW_CNT == number of PCIADDSUB registers'. For example, CFG_PCIM_MEM_END can lead to SZ_512M with CFG_PCIM_WINDOW_SZ=16M and PHYS -> PCI substitution should be configured accordingly. Of course this is not supported by the PCI IP but the pci-common.c code will not require any change when there is such a modification in IP. Maybe I should define MAX_PCIADDSUB_REGS and make sure CFG_PCIM_WINDOW_CNT doesn't exceed it? [...] > > +/* > > + * Provide the virtual address of PCI Backend register at specified > offset. Uses > > + * SoC specific PCI register base for address calculation. > > + */ > > +#define PCI_REGV(reg) (IO_ADDRESS(PCICTL_REG_BASE) + reg) > > Rather than use IO_ADDRESS() here, please use ioremap during init. > I will do that. > > +struct pcihost_bar_cfg { > > + u32 bar_window_phys; > > + u32 bar_window_size; > > +}; > > + > > +/* Forward declarations */ > > +extern int __init ti_pci_setup(int nr, struct pci_sys_data *); > > +extern struct pci_bus * __init ti_pci_scan(int nr, struct pci_sys_data > *); > > +extern struct pcihost_bar_cfg bar_cfg[6]; > > + > > +#endif /* !__ARCH_PCI_H */ > > Kevin Thanks, Hemant _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
