"Pedanekar, Hemant" <hema...@ti.com> writes:
> Kevin,
>
> Thanks for the review. Please see my comments below.
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
>> Sent: Friday, October 16, 2009 4:17 AM
>> To: Pedanekar, Hemant
>> Cc: davinci-linux-open-source@linux.davincidsp.com
>> Subject: Re: [PATCH v2 2/4] davinci: dm646x: Add PCI host controller
>> driver
>>
>> Hemant Pedanekar <hema...@ti.com> 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 <hema...@ti.com>
>>
>> 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?
I think the PCI abort will be enough.
> [...]
>
>> > +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?
I was thinking you should probably create a
platform_driver/platform_device and let the board code pass a default
BAR mapping via platform_data.
> [...]
>> > +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?
Yes, you're right.
> [...]
>
>> > + /* 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?
Based on my cursory glace at the DM646x PCI spec, it seemed that the
physical window size was limited to 256M. Since the user could change
the the number of windows etc., it seemed like some extra protection
was needed so it couldn't exceed the physcial memory map. If it can
actually exceed that due to the other settings, then some other sanity
check would be useful.
Kevin
> [...]
>> > +/*
>> > + * 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
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
