lkml.org 
[lkml]   [2014]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 1/4] pci: APM X-Gene PCIe controller driver
    Date
    On Thursday 06 March 2014, Tanmay Inamdar wrote:

    > +static inline void xgene_pcie_cfg_out16(void __iomem *addr, u16 val)
    > +{
    > + u64 temp_addr = (u64)addr & ~0x3;

    Please use 'unsigned long' as the type for calculations like this one,
    to make the code more portable. You mentioned before that the same PCI
    host controller is used on some ppc4xx, and we may want to share the
    code later.

    > +static int xgene_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
    > + int offset, int len, u32 *val)
    > +{
    > + struct xgene_pcie_port *port = bus->sysdata;
    > + void __iomem *addr;
    > + u8 val8;
    > + u16 val16;
    > +
    > + if ((pci_is_root_bus(bus) && devfn != 0) || !port->link_up)
    > + return PCIBIOS_DEVICE_NOT_FOUND;
    > +
    > + xgene_pcie_set_rtdid_reg(bus, devfn);
    > + addr = xgene_pcie_get_cfg_base(bus);
    > + switch (len) {
    > + case 1:
    > + xgene_pcie_cfg_in8(addr + offset, &val8);
    > + *val = val8;
    > + break;

    Actually it would be better to just pass both addr and offset
    down into the low-level accessors and then do the calculation
    on 'offset', which is already a scalar.

    > +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port,
    > + u32 *lanes, u32 *speed)
    > +{
    > + void __iomem *csr_base = port->csr_base;
    > + u32 val32;
    > + u64 start_time, time;
    > +
    > + /*
    > + * A component enters the LTSSM Detect state within
    > + * 20ms of the end of fundamental core reset.
    > + */
    > + msleep(XGENE_LTSSM_DETECT_WAIT);
    > + port->link_up = 0;
    > + start_time = jiffies;
    > + do {
    > + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
    > + if (val32 & LINK_UP_MASK) {
    > + port->link_up = 1;
    > + *speed = PIPE_PHY_RATE_RD(val32);
    > + val32 = readl(csr_base + BRIDGE_STATUS_0);
    > + *lanes = val32 >> 26;
    > + break;
    > + }
    > + msleep(1);
    > + time = jiffies_to_msecs(jiffies - start_time);
    > + } while (time <= XGENE_LTSSM_L0_WAIT);
    > +}

    This can be written ina simpler way using 'time_before()'.

    > +/* Return 0 on success */
    > +static int xgene_pcie_init_ecc(struct xgene_pcie_port *port)
    > +{
    > + void __iomem *csr_base = port->csr_base;
    > + u64 start_time, time = 0;
    > + u32 val;
    > +
    > + val = readl(csr_base + MEM_RAM_SHUTDOWN);
    > + if (!val)
    > + return 0;
    > + writel(0x0, csr_base + MEM_RAM_SHUTDOWN);
    > + start_time = jiffies;
    > + do {
    > + val = readl(csr_base + BLOCK_MEM_RDY);
    > + if (val == BLOCK_MEM_RDY_VAL)
    > + break;
    > + msleep(1);
    > + time = jiffies_to_msecs(jiffies - start_time);
    > + } while (time < XGENE_PCIE_ECC_TIMEOUT);

    Same here.

    > +static int xgene_pcie_init_port(struct xgene_pcie_port *port)
    > +{
    > + int rc;
    > +
    > + port->clk = clk_get(port->dev, NULL);
    > + if (IS_ERR_OR_NULL(port->clk)) {
    > + dev_err(port->dev, "clock not available\n");
    > + return -ENODEV;
    > + }

    Practically every use of IS_ERR_OR_NULL() is a bug, don't do that.
    NULL is a valid return code from clk_get(), and should not be
    treated as an error.


    > +static int xgene_pcie_map_ranges(struct xgene_pcie_port *port,
    > + struct pci_host_bridge *bridge,
    > + u64 cfg_addr)
    > +{
    > + struct device *dev = port->dev;
    > + struct pci_host_bridge_window *window;
    > +
    > + list_for_each_entry(window, &bridge->windows, list) {
    > + struct resource *res = window->res;
    > + u64 restype = resource_type(res);
    > + dev_dbg(port->dev, "0x%08lx 0x%016llx...0x%016llx\n",
    > + res->flags, res->start, res->end);
    > +
    > + switch (restype) {
    > + case IORESOURCE_IO:
    > + xgene_pcie_setup_ob_reg(port, res, OMR2BARL,
    > + bridge->io_base);
    > + BUG_ON(pci_ioremap_io(res, bridge->io_base) < 0);
    > + break;

    No need to BUG_ON() here, this is not a fatal condition, just
    don't register the I/O space resource if this fails.

    I think as the PCI base support patch series evolves, you will actually
    not have to do this at all.

    Arnd


    \
     
     \ /
      Last update: 2014-03-14 14:01    [W:2.547 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site