Date:	Fri, 14 Mar 2014 20:29:13 -0700
From:	Tanmay Inamdar <tinamdar@....com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Rob Landley <rob@...dley.net>,
	Liviu Dudau <liviu.dudau@....com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, patches <patches@....com>,
	Jon Masters <jcm@...hat.com>
Subject: Re: [PATCH v4 1/4] pci: APM X-Gene PCIe controller driver

Thanks for the review and comments. I will incorporate the comments
from you and Jingoo Han in next version.

-Tanmay

On Fri, Mar 14, 2014 at 5:18 AM, Arnd Bergmann <arnd@...db.de> wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists