Re: [PATCH 1/1 v2 ] PCI: Mobiveil: Add Mobiveil PCIe Host Bridge IP driver

[Subrahmanya Lingappa <l.subrahmanya@mobiveil.co.in>]

Bjorn,
Thanks for the review.
I will comeback with the next version soon.

~subbu

On Fri, Nov 10, 2017 at 5:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Nov 09, 2017 at 05:33:03PM +0530, subrahmanya_lingappa wrote:
>> From f0eef95dec090bd211b398dee52c31c18212be9a Mon Sep 17 00:00:00 2001
>> From: Subrahmanya Lingappa <l.subrahmanya@mobiveil.co.in>
>> Date: Thu, 9 Nov 2017 01:46:10 -0500
>> Subject: [PATCH 1/1 v2 ] PCI: Mobiveil: Add Mobiveil PCIe Host
>> Bridge IP driver
>>
>> This is the driver for Mobiveil  AXI PCIe Host Bridge Soft IP
>>
>> Cc: bhelgaas@google.com
>>
>> Signed-off-by: Subrahmanya Lingappa <l.subrahmanya@mobiveil.co.in>
>> ---
>>  .../devicetree/bindings/pci/mobiveil-pcie.txt      |   67 ++
>>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>>  MAINTAINERS                                        |    8 +
>>  drivers/pci/host/Kconfig                           |   10 +
>>  drivers/pci/host/Makefile                          |    1 +
>>  drivers/pci/host/pcie-mobiveil.c                   | 1144
>> ++++++++++++++++++++
>>  6 files changed, 1231 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/mobiveil-pcie.txt
>>  create mode 100644 drivers/pci/host/pcie-mobiveil.c
>>
>> diff --git a/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt
>> b/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt
>> new file mode 100644
>> index 0000000..2426cab
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt
>> @@ -0,0 +1,67 @@
>> +* mobiveil AXI PCIe Root Port Bridge DT description
>
> s/mobiveil/Mobiveil/
>
>> +
>> +Required properties:
>> +- #address-cells: Address representation for root ports, set to <3>
>> +- #size-cells: Size representation for root ports, set to <2>
>> +- #interrupt-cells: specifies the number of cells needed to encode an
>> +     interrupt source. The value must be 1.
>> +- compatible: Should contain "mbvl,axi-pcie-host-1.00"
>> +- reg: Should contain AXI PCIe registers location and length
>> +- device_type: must be "pci"
>> +- interrupts: Should contain AXI PCIe interrupt
>> +- interrupt-map-mask,
>> +  interrupt-map: standard PCI properties to define the mapping of the
>> +     PCI interface to interrupt numbers.
>> +- ranges: ranges for the PCI memory regions (I/O space region is not
>> +     supported by hardware)
>> +     Please refer to the standard PCI bus binding document for a more
>> +     detailed explanation
>> +
>> +Optional properties for Zynq/Microblaze:
>
> I don't think this is specific to Zynq/Microblaze.  I'd remove that
> text.
>
>> +- bus-range: PCI bus numbers covered
>> +
>> +Interrupt controller child node
>> ++++++++++++++++++++++++++++++++
>> +Required properties:
>> +- interrupt-controller: identifies the node as an interrupt controller
>> +- #address-cells: specifies the number of cells needed to encode an
>> +     address. The value must be 0.
>> +- #interrupt-cells: specifies the number of cells needed to encode an
>> +     interrupt source. The value must be 1.
>> +
>> +NOTE:
>> +The core provides a single interrupt for both INTx/MSI messages. So,
>> +created a interrupt controller node to support 'interrupt-map' DT
>> +functionality.  The driver will create an IRQ domain for this map, decode
>> +the four INTx interrupts in ISR and route them to this domain.
>> +
>> +
>> +Example:
>> +++++++++
>> +     pci_express: axi-pcie@a0000000 {
>> +             #address-cells = <3>;
>> +             #size-cells = <2>;
>> +             #interrupt-cells = <1>;
>> +             compatible = "mbvl,axi-pcie-host-1.00";
>> +             reg =   < 0xa0000000 0x1000
>> +                               0xb0000000 0x00010000
>> +                          0xFF000000 0x200000
>> +                          0xb0010000 0x1000 >;
>
> It'd be nice to format this as a table to show the natural structure,
> as you did for interrupt-map below.
>
> It looks like the conventional style omits the spaces after "<" and
> before ">".
>
>> +             reg-names = "config_axi_slave",
>> +                            "csr_axi_slave",
>> +                            "gpio_slave",
>> +                            "apb_csr";
>> +
>> +             device_type = "pci";
>> +             interrupt-parent = <&gic>;
>> +             interrupts = < 0 89 4 >;
>> +             interrupt-controller;
>> +             interrupt-map-mask = <0 0 0 7>;
>> +             interrupt-map = <0 0 0 1 &pci_express 1>,
>> +                             <0 0 0 2 &pci_express 2>,
>> +                             <0 0 0 3 &pci_express 3>,
>> +                             <0 0 0 4 &pci_express 4>;
>> +             ranges = < 0x83000000 0 0x00000000 0xa8000000 0 0x8000000>;
>> +
>> +     };
>> +
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index f0a48ea..29172e0 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -179,6 +179,7 @@ msi       Micro-Star International Co. Ltd.
>>  mti  Imagination Technologies Ltd. (formerly MIPS Technologies Inc.)
>>  mundoreader  Mundo Reader S.L.
>>  murata       Murata Manufacturing Co., Ltd.
>> +mbvl Mobiveil Inc.
>
> Please follow existing indentation style, i.e., looks like there
> should be a tab after "mbvl".
>
>>  mxicy        Macronix International Co., Ltd.
>>  national     National Semiconductor
>>  nec  NEC LCD Technologies, Ltd.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 63cefa6..6f3212e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9336,6 +9336,14 @@ F:
>> Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>  F:   drivers/pci/host/pci-host-common.c
>>  F:   drivers/pci/host/pci-host-generic.c
>>
>> +PCI DRIVER FOR ALTERA PCIE IP
>> +M:   Subrahmanya Lingappa <l.subrahmanya@mobiveil.co.in>
>> +L:   linux-pci@vger.kernel.org
>> +S:   Supported
>> +F:   Documentation/devicetree/bindings/pci/mobiveil-pcie.txt
>> +F:   drivers/pci/host/pcie-mobiveil.c
>
> Thanks for this; people usually forget to include it.
>
>> +
>> +
>>  PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
>>  M:   Keith Busch <keith.busch@intel.com>
>>  L:   linux-pci@vger.kernel.org
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 6f1de4f..c6a1209 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -37,6 +37,15 @@ config PCIE_XILINX_NWL
>>        or End Point. The current option selection will only
>>        support root port enabling.
>>
>> +config PCIE_MOBIVEIL
>> +     bool "Mobiveil AXI PCIe host bridge support"
>> +     depends on ARCH_ZYNQMP
>
> As far as I know; there's nothing in the *code* that requires
> ARCH_ZYNQMP.  Can you add "|| COMPILE_TEST" here?  That way we can get
> better compile test coverage.
>
>> +     depends on PCI_MSI_IRQ_DOMAIN
>> +     help
>> +       Say 'Y' here if you want kernel to support the Mobiveil AXI PCIe
>> +       Host Bridge driver.
>> +
>> +
>>  config PCIE_DW_PLAT
>>       bool "Platform bus based DesignWare PCIe Controller"
>>       depends on PCI_MSI_IRQ_DOMAIN
>> @@ -103,6 +112,7 @@ config PCI_HOST_GENERIC
>>         Say Y here if you want to support a simple generic PCI host
>>         controller, such as the one emulated by kvmtool.
>>
>> +
>
> Spurious blank line addition; remove it.
>
>>  config PCIE_SPEAR13XX
>>       bool "STMicroelectronics SPEAr PCIe controller"
>>       depends on ARCH_SPEAR13XX
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 9b113c2..0f2b5f5 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o
>> pci-keystone.o
>>  obj-$(CONFIG_PCIE_XDMA_PL) += pcie-xdma-pl.o
>>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
>> +obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
>>  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>> diff --git a/drivers/pci/host/pcie-mobiveil.c
>> b/drivers/pci/host/pcie-mobiveil.c
>> new file mode 100644
>> index 0000000..3c1edf6
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-mobiveil.c
>> @@ -0,0 +1,1144 @@
>> +/*
>> + * PCIe host controller driver for Mobiveil PCIe Host controller
>> + *
>> + * Copyright mobiveil Corporation (C) 2013-2017. All rights reserved
>
> s/mobiveil/Mobiveil/  (Capitalize it consistently in English text)
>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
>> License for
>
> "Patch" complains that this patch is corrupted, I think because this
> line is wrapped.  Make sure your mailer doesn't wrap any lines.
>
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public
>> License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irq.h>
>> +#include <linux/msi.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/kernel.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/init.h>
>> +#include <linux/of_platform.h>
>> +
>> +/*Register Offsets and Bit Positions*/
>
> Add space after "/*" and before "*/" (also occurs other places).
>
>> +#define BAR_MAP_BASE                0x00000000
>> +#define WIN1_BAR_MAP_DISTANCE       0x8000000
>> +#define WIN2_BAR_MAP_DISTANCE       0x4000000
>> +#define WIN3_BAR_MAP_DISTANCE       0x8000000
>
> WIN2_BAR_MAP_DISTANCE and WIN3_BAR_MAP_DISTANCE are unused.  In
> general, omit things that are unused because they add clutter and give
> the false impression that they *are* used.  If you need them in the
> future, you can easily add them.
>
>> +#define WIN_NUM_0                   0
>> +#define WIN_NUM_1                   1
>> +#define WIN_NUM_2                   2
>> +#define WIN_NUM_3                   3
>
> WIN_NUM_2 and WIN_NUM_3 are unused.
>
>> +#define PAB_INTA_POS                 5
>> +#define OP_READ                              0
>> +#define OP_WRITE                     1
>> +
>> +#define WIN_1_SIZE                  (1024 * 1024)
>> +#define WIN_1_BASE                  (0xa0000000)
>> +#define WIN_2_SIZE                  (128 * 1024 * 1024)
>> +#define WIN_2_BASE                  (WIN_1_BASE + WIN_1_SIZE)
>
> Of the above, only WIN_1_BASE is used.  And it's a single value and
> doesn't need parentheses around it.
>
>> +#define IB_WIN_SIZE_KB              (1*1024*1024*1024)
>
> This is a pretty large number of KB.  Is it really accurate?
> IB_WIN_SIZE_KB = 1G.  1G KB would be 1024GB.
>
>> +#define OB_CFG_WIN_SIZE_KB              (0x0010000/1024)     // 64KB
>> +#define OB_MEM_WIN_SIZE_KB              (0x8000000/1024)     // 128MB
>
> These are confusing ways to write "64" and "128*1024".
>
>> +/* ltssm_state_status*/
>> +#define LTSSM_STATE_STATUS_REG               0x0404
>> +#define LTSSM_STATUS_CODE_MASK               0x3F
>> +#define LTSSM_STATUS_CODE            0x2D    /* LTSSM Status Code L0 */
>
> This name (LTSSM_STATUS_CODE) is not very descriptive.  Presumably
> there are *other* status codes, too, so this should be something like
> LTSSM_STATUS_L0.
>
>> +#define PAB_CAP_REG                 0x0804
>
> Unused.
>
>> +#define PAB_CTRL_REG                0x0808
>> +#define AMBA_PIO_ENABLE_BIT          0
>> +#define PEX_PIO_ENABLE_BIT           1
>
> AMBA_PIO_ENABLE_BIT and PEX_PIO_ENABLE_BIT are unused.
>
> Looks like you indent the field names below, but not above?  Pick one.
> I think the indentation is useful.  It's also useful if the field
> names are related to the register names.  See
> include/uapi/linux/pci_regs.h for examples.
>
>> +#define PAB_AXI_PIO_CTRL_REG(engine)         (0x0840 + 0x10*engine)
>
> You only ever use engine 0, so not clear that the "engine" argument is
> useful.
>
>> +#define  PIO_ENABLE_BIT              0
>
> Unused.
>
>> +#define  CFG_WINDOW_TYPE             0
>> +#define  IO_WINDOW_TYPE              1
>
> Unused.
>
>> +#define  MEM_WINDOW_TYPE             2
>> +
>> +#define PAB_PEX_PIO_CTRL_REG         0x08C0
>> +#define PAB_INTP_AMBA_MISC_ENB               0x0B0C
>> +#define PAB_INTP_AMBA_MISC_STAT              0x0B1C
>> +#define  PAB_INTP_INTX_MASK          0x1E0   /* INTx(A-D) */
>> +#define  PAB_INTP_MSI_MASK           0x8
>> +
>> +#define PAB_AXI_AMAP_CTRL_REG(win_num)  (0x0BA0 + 0x10*win_num)
>> +#define PAB_EXT_AXI_AMAP_SIZE(win_num)  (0xBAF0 + 0x4*win_num)
>> +#define  ENABLE_BIT          0
>> +#define  TYPE_BIT            1
>> +#define  AXI_WINDOW_SIZE_BIT         10
>> +
>> +#define PAB_AXI_AMAP_AXI_WIN_REG(win_num)       (0x0BA4 + 0x10*win_num)
>> +#define  AXI_WINDOW_BASE_BIT         2
>> +#define PAB_EXT_AXI_AMAP_AXI_WIN_REG(win_num)   (0x80A0 + 0x4*win_num)
>
> Unused.
>
>> +#define PAB_AXI_AMAP_PEX_WIN_L_REG(win_num)     (0x0BA8 + 0x10*win_num)
>> +#define  BUS_BIT             24
>> +#define  DEVICE_BIT          19
>> +#define  FUNCTION_BIT                16
>> +#define  REGISTER_BIT                0
>
> Unused.
>
>> +#define PAB_AXI_AMAP_PEX_WIN_H_REG(win_num) (0x0BAC + 0x10*win_num)
>> +#define PAB_INTP_AXI_PIO_ENB_REG        0xB00
>> +#define PAB_INTP_AXI_PIO_STAT_REG       0xB10
>> +#define PAB_INTP_AXI_PIO_VENID_REG      0x470
>> +#define PAB_INTP_AXI_PIO_DEVID_REG      0x472
>
> Above four are unused.
>
>> +#define PAB_INTP_AXI_PIO_CLASS_REG      0x474
>> +
>> +#define PAB_PEX_AMAP_CTRL(win_num)      (0x4BA0 + (0x10*win_num))
>> +#define PAB_EXT_PEX_AMAP_SIZEN(win_num) (0xBEF0 + (0x4*win_num))
>> +#define PAB_PEX_AMAP_AXI_WIN(win_num)   (0x4BA4 + (0x10*win_num))
>> +#define PAB_PEX_AMAP_PEX_WIN_L(win_num) (0x4BA8 + (0x10*win_num))
>> +#define PAB_PEX_AMAP_PEX_WIN_H(win_num) (0x4BAC + (0x10*win_num))
>> +
>> +#define INTX_NUM                        4
>
> Use the existing PCI_NUM_INTX definition instead.
>
>> +#define MOBIVEIL_NUM_MSI_IRQS           16
>> +
>> +#define MSI_BASE_LO_OFFSET           0x04
>> +#define MSI_BASE_HI_OFFSET           0x08
>> +#define MSI_SIZE_OFFSET              0x0c
>> +#define MSI_ENABLE_OFFSET            0x14
>> +#define MSI_ENABLE_BIT_POS           0
>> +#define MSI_STATUS_OFFSET            0x18
>> +#define MSI_STATUS_BIT_POS           0
>> +#define MSI_OCCUPANCY_OFFSET         0x1c
>> +#define MSI_DATA_OFFSET              0x20
>> +#define MSI_ADDR_L_OFFSET            0x24
>> +#define MSI_ADDR_H_OFFSET            0x28
>> +
>> +#define ilog2_u32(num) (__ffs(num)-1)
>> +
>> +/*   local prototypes */
>
> Don't indent your comments with a tab.  One space is sufficient and
> typical.  And this prototype isn't needed anyway and should be
> removed.
>
>> +static irqreturn_t mobiveil_pcie_isr(int irq, void *data);
>> +
>> +/*
>> + *   PCIe port information
>> + */
>> +struct mobiveil_pcie {
>> +     /* platform device pointer */
>> +     struct platform_device *pdev;
>
> Please put the comments on the same line as the member, e.g.,
>
>   struct platform_device *pdev;    /* platform device pointer */
>
> Actually, that one was a bad example, because that comment adds no
> information and should be removed completely.  *Most* of the comments
> below should be removed.
>
>> +     /* memory  mapped register base for endpoint config access */
>> +     void __iomem *config_axi_slave_base;
>> +     /* memory mapped register  base for root port config access */
>> +     void __iomem *csr_axi_slave_base;
>> +     /* memory mapped GPIO register base for root port config access */
>> +     void __iomem *gpio_slave_base;
>> +     /* memory mapped GPIO register base for root port config access */
>> +     void __iomem *apb_csr_base;
>> +     /* irq domain associated with root port */
>> +     struct irq_domain *irq_domain;
>> +     /* bus range resource */
>> +     struct resource bus_range;
>> +     /* head pointer for all the enumerated resources */
>> +     struct list_head resources;
>> +     /*  Virtual and physical addresses of the MSI data */
>> +     int *msi_pages;
>> +     int *msi_pages_phys;
>> +     /* Root port bus number */
>> +     u8 root_bus_nr;
>> +     /* IRQ number */
>> +     int irq;
>> +};
>> +
>> +/*
>> + *   union pab_pex_amap_ctrl_t - PAB_PEX_AMAP register bitfields
>> + */
>> +__packed union pab_pex_amap_ctrl_t{
>> +     int dw;
>> +
>> +     __packed struct {
>> +
>> +             int enable:1;
>> +             int type:2;
>> +             int no_snoop_ov_en:1;
>> +             int no_snoop:1;
>> +             int rsvd:5;
>> +             int size:22;
>> +     };
>> +};
>
> Unions are klunky and unconventional.  Just #define some fields like
> you do above.
>
>> +/*
>> + *   union pab_ctrl_t - PAB_CTRL register bitfields
>> + */
>> +__packed union pab_ctrl_t{
>> +     int dw;
>> +
>> +     __packed struct {
>> +
>> +             int amba_pio:1;
>> +             int pex_pio:1;
>> +             int wdma:1;
>> +             int rdma:1;
>> +             int axi_max_burst_len:2;
>> +             int rsvd:1;
>> +             int dma_pio_arb:1;
>> +             int prefetch_depth:2;
>> +             int mrrs:3;
>> +             int pg_sel:6;
>> +             int func_sel:9;
>> +             int res1:1;
>> +             int msi_sw_ctrl_en:1;
>> +             int res2:2;
>> +     };
>> +};
>> +
>> +/*   global variables  */
>> +
>> +u32 serving_interrupt = 0, max_msi_allocated = 0;
>
> Making these global is bad style because it means you're limited to a
> single device instance.
>
>> +u32 msi_ints = 0, msi_msgs = 0;
>
> Not used at all; remove these.
>
>> +static DECLARE_BITMAP(msi_irq_in_use, MOBIVEIL_NUM_MSI_IRQS);
>
> This should be a member of struct mobiveil_pcie, not a global.
>
>> +
>> +/*
>> + *   csr_writel - routine to write one DWORD to memory mapper register
>
> s/mapper/mapped/ (also below)
>
> Actually, I think these function comments are superfluous, too.  They
> take up space and don't add any useful information.
>
>> + *
>> + *   @pcie :   pointer to root port
>> + *   @value:   value to be written to register
>> + *   @reg  :   register offset
>> + */
>> +static inline void csr_writel(struct mobiveil_pcie *pcie, const u32 value,
>> +                           const u32 reg)
>> +{
>> +     writel_relaxed(value, pcie->csr_axi_slave_base + reg);
>> +}
>> +
>> +/*
>> + *   csr_readl - routine to read one DWORD from memory mapper register
>> + *
>> + *   @pcie :    pointer to root port
>> + *   @reg  :    register offset
>> + */
>> +
>> +static inline u32 csr_readl(struct mobiveil_pcie *pcie, const u32 reg)
>> +{
>> +     return readl_relaxed(pcie->csr_axi_slave_base + reg);
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_link_is_up - routine to check if PCIe link is up
>> + *
>> + *   @pcie :    pointer to root port
>> + */
>> +
>> +static bool mobiveil_pcie_link_is_up(struct mobiveil_pcie *pcie)
>
> Rename to mobiveil_pcie_link_up().  The altera and xilinx drivers are
> the oddballs here and should be renamed.  All the others use
> .*_pcie_link_up().
>
>> +{
>> +     return (csr_readl(pcie, LTSSM_STATE_STATUS_REG) &
>> +             LTSSM_STATUS_CODE_MASK) == LTSSM_STATUS_CODE;
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_valid_device - routine to check if a valid device/function
>> + *   is present on the bu
>> + *
>> + *   @pcie :    pointer to root por
>> + */
>
> I'd say this comment is pointless, but if you keep it,
> s/bu/bus/
> s/por/port/
>
>> +static bool mobiveil_pcie_valid_device(struct pci_bus *bus, u32 devfn)
>> +{
>> +     struct mobiveil_pcie *pcie = bus->sysdata;
>> +
>> +     /* Check if link is up when trying to access downstream ports */
>> +     if (bus->number != pcie->root_bus_nr)
>> +             if (!mobiveil_pcie_link_is_up(pcie))
>> +                     return false;
>> +
>> +     /* Only one device down on each root port */
>> +     if ((bus->number == pcie->root_bus_nr) && (devfn > 0))
>> +             return false;
>> +
>> +     /* Do not read more than one device on the bus directly
>> +      * attached to RC
>> +      */
>> +     if ((bus->primary == pcie->root_bus_nr) && (devfn > 0))
>> +             return false;
>> +
>> +     return true;
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_map_bus - routine to get the configuration base of either
>> + *   root port or endpoin
>> + *
>> + *   @bus  :   pointer to local bu
>> + *   @devfn:   variable containing the device and function number
>> + *   @where:   register offse
>> + */
>
> Again the comment is unnecessary, but,
> s/endpoin/endpoint/
> s/bu/bus/
> s/offse/offset/
>
>> +static void __iomem *mobiveil_pcie_map_bus(struct pci_bus *bus,
>> +                                        u32 devfn, int where)
>> +{
>> +     struct mobiveil_pcie *pcie = bus->sysdata;
>> +     void __iomem *addr = NULL;
>> +
>> +     if (!mobiveil_pcie_valid_device(bus, devfn))
>> +             return NULL;
>> +
>> +     if (bus->number == pcie->root_bus_nr) {
>> +             /* RC config access (in CSR space) */
>> +             addr = pcie->csr_axi_slave_base + where;
>
>   if (bus->number == pcie->root_bus_nr)
>     return pcie->csr_axi_slave_base + where;
>
>   /* Relies on pci_lock serialization */
>   value = csr_readl(pcie, PAB_AXI_AMAP_PEX_WIN_L_REG(0));
>   ...
>   return pcie->config_axi_slave_base + where;
>
>> +     } else {
>> +             /* EP config access (in Config/APIO space) */
>> +             u32 value;
>> +
>> +             /* Program PEX Address base (31..16 bits) with appropriate value
>> +              * (BDF) in PAB_AXI_AMAP_PEX_WIN_L0 Register
>> +              */
>> +             value = csr_readl(pcie, PAB_AXI_AMAP_PEX_WIN_L_REG(0));
>> +             csr_writel(pcie,
>> +                        bus->
>> +                        number << BUS_BIT | (devfn >> 3) << DEVICE_BIT |
>> +                        (devfn & 7) << FUNCTION_BIT,
>> +                        PAB_AXI_AMAP_PEX_WIN_L_REG(0));
>> +             addr = pcie->config_axi_slave_base + where;
>> +     }
>> +     return addr;
>> +}
>> +
>> +/*   PCIe operations */
>
> Unnecessary comment.
>
>> +static struct pci_ops mobiveil_pcie_ops = {
>> +     .map_bus = mobiveil_pcie_map_bus,
>> +     .read = pci_generic_config_read,
>> +     .write = pci_generic_config_write,
>> +};
>> +
>> +/*
>> + *   mobiveil_pcie_isr - interrupt handler for root complex
>> + *
>> + *   @irq    : IRQ numbe
>> + *   @data   : pointer to driver specific data
>> + */
>
> Unnecessary comment, but
> s/numbe/number/
>
>> +static irqreturn_t mobiveil_pcie_isr(int irq, void *data)
>> +{
>> +     struct mobiveil_pcie *pcie;
>
>   struct mobiveil_pcie *pcie = data;
>
>> +     u32 status, shifted_status, status2;
>> +     u32 bit1 = 0, virq = 0;
>
> No need to initialize virq.
>
>> +     u32 val, mask;
>> +
>> +     if (serving_interrupt == 0)
>> +             serving_interrupt = 1;
>> +     else
>> +             return IRQ_NONE;
>
> Bogus.  I don't know what you're doing here, but you can't use a
> global variable like this.
>
>> +
>> +     pcie = (struct mobiveil_pcie *)data;
>> +
>> +     val = csr_readl(pcie, PAB_INTP_AMBA_MISC_STAT);
>> +     status2 = readl(pcie->apb_csr_base + MSI_STATUS_OFFSET);
>> +     mask = csr_readl(pcie, PAB_INTP_AMBA_MISC_ENB);
>> +     status = val & mask;
>> +
>> +     if (status & PAB_INTP_INTX_MASK) {
>> +             while ((shifted_status =
>> +                     (csr_readl(pcie, PAB_INTP_AMBA_MISC_STAT) >>
>> +                      PAB_INTA_POS)) != 0) {
>> +                     for_each_set_bit(bit1, &shifted_status, INTX_NUM) {
>> +                             /* clear interrupts */
>> +                             csr_writel(pcie, shifted_status << PAB_INTA_POS,
>> +                                        PAB_INTP_AMBA_MISC_STAT);
>> +
>> +                             virq =
>> +                                 irq_find_mapping(pcie->irq_domain,
>> +                                                  bit1 + 1);
>> +                             if (virq)
>> +                                     generic_handle_irq(virq);
>> +                             else
>> +                                     dev_err(&pcie->pdev->dev,
>> +                                             "unexpected IRQ, INT%d\n",
>> +                                             bit1);
>> +
>> +                     }
>> +                     shifted_status = 0;
>> +             }
>> +     }
>> +
>> +     if ((status & PAB_INTP_MSI_MASK)
>> +         || (status2 & (1 << MSI_STATUS_BIT_POS))) {
>> +             u32 fifo_occ = 0;
>> +             u32 msi_addr_l = 0, msi_addr_h = 0, msi_data = 0;
>
> No need to initialize any of these variables.
>
>> +
>> +             do {
>> +                     fifo_occ = readl(pcie->apb_csr_base
>> +                                     + MSI_OCCUPANCY_OFFSET);
>> +                     msi_data = readl(pcie->apb_csr_base
>> +                                     + MSI_DATA_OFFSET);
>> +                     msi_addr_l = readl(pcie->apb_csr_base
>> +                                     + MSI_ADDR_L_OFFSET);
>> +                     msi_addr_h = readl(pcie->apb_csr_base
>> +                                     + MSI_ADDR_H_OFFSET);
>> +                     /* Handle MSI */
>> +                     if (msi_data)
>> +                             generic_handle_irq(msi_data);
>> +                     else
>> +                             dev_err(&pcie->pdev->dev, "MSI data null\n");
>> +
>> +                     status2 = readl(pcie->apb_csr_base + MSI_STATUS_OFFSET);
>> +             } while ((status2 & (1 << MSI_STATUS_BIT_POS)));
>> +     }
>> +
>> +     csr_writel(pcie, status, PAB_INTP_AMBA_MISC_STAT);
>> +     serving_interrupt = 0;
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +/*
>> + *   map_resource_base - routine to parse the device tree memory resource
>> + *   and remap it; returns the remapped address.
>> + *
>> + *   @pcie       : pointer to root port
>> + *   @res_name   : pointer to the device tree resource name
>> + *
>> + */
>> +void __iomem *map_resource_base(struct mobiveil_pcie *pcie, s8 *res_name)
>
> Must be static.  Actually, I think you should just inline this
> function at its callers.  The common pattern is:
>
>   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config_axi_slave");
>   pcie->config_axi_slave_base = devm_ioremap_resource(dev, res);
>   if (IS_ERR(pcie->config_axi_slave_base))
>     return PTR_ERR(pcie->config_axi_slave_base);
>
> If you do that at the caller, it's shorter and clearer than what you
> have here.
>
>> +{
>> +     struct platform_device *pdev = pcie->pdev;
>> +     struct resource *res;
>> +     void __iomem *res_base;
>> +
>> +     /* read  resource */
>
> Unnecessary comments.
>
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name);
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "no %s memory resource defined\n",
>> +                     res_name);
>> +             return res;
>> +     }
>> +
>> +     /* remap  resource */
>> +     res_base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(res_base)) {
>> +             dev_err(&pdev->dev, "failed to map %s memory\n", res_name);
>> +             return res_base;
>> +     }
>> +
>> +     return res_base;
>> +}
>> +
>> +/*
>> + *   remap_reg_base - routine to parse the device tree registers base
>> + *   resource and remap it.
>> + *
>> + *   @pcie       : pointer to root port
>> + *   @res_name   : pointer to the device tree resource name
>> + *
>> + *   returns the remapped address
>> + *
>> + */
>> +void __iomem *remap_reg_base(struct mobiveil_pcie *pcie, s8 *res_name)
>> +{
>> +
>> +     struct platform_device *pdev = pcie->pdev;
>> +     struct resource *res;
>> +     void __iomem *res_base;
>> +
>> +     /* read  resource */
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name);
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "no %s memory resource defined\n",
>> +                     res_name);
>> +             return res;
>> +     }
>> +
>> +     /* remap  resource */
>> +     res_base = ioremap_nocache(res->start, resource_size(res));
>
> Inline this as above, and use devm_ioremap_nocache(), not
> ioremap_nocache().
>
>> +     if (IS_ERR(res_base)) {
>> +             dev_err(&pdev->dev, "failed to map %s memory\n", res_name);
>> +             return res_base;
>> +     }
>> +
>> +     return res_base;
>> +}
>> +
>> +/*
>> + *   pcie_request_irq - Routrine to parse the device tree and map the
>> + *   IRQ number.
>> + *
>> + *   @pcie       : pointer to root port
>> + *
>> + *   returns the mapped IRQ number
>> + */
>
> Remove comment, or at least
> s/Routrine/routine/
>
>> +int pcie_request_irq(struct mobiveil_pcie *pcie)
>
> Inline into mobiveil_pcie_parse_dt(), as xilinx_pcie_parse_dt() does.
> Use the same error checking as xilinx_pcie_parse_dt() does.  Or if
> that one is buggy, fix them both.
>
>> +{
>> +     struct platform_device *pdev = pcie->pdev;
>> +     struct device *dev = &pcie->pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     int ret = 0, irq = 0;
>> +
>> +     /* map IRQ */
>> +     irq = irq_of_parse_and_map(node, 0);
>> +     if (irq <= 0) {
>> +             dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq);
>> +             return -EINVAL;
>> +     }
>> +     ret = devm_request_irq(&pdev->dev, irq, mobiveil_pcie_isr,
>> +                            IRQF_SHARED | IRQF_NO_THREAD,
>> +                            "mobiveil-pcie", pcie);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "unable to request irq %d\n", irq);
>
> s/irq/IRQ/ in the error message.
>
>> +             return ret;
>> +
>> +     }
>> +
>> +     return irq;
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_parse_dt - routine to parse the device tree structure and
>> + *   extract the resource info
>> + *
>> + *   @pcie :    pointer to root port
>> + */
>> +
>> +static int mobiveil_pcie_parse_dt(struct mobiveil_pcie *pcie)
>> +{
>> +     struct device *dev = &pcie->pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     const s8 *type;
>> +
>> +     type = of_get_property(node, "device_type", NULL);
>> +     if (!type || strcmp(type, "pci")) {
>> +             dev_err(dev, "invalid \"device_type\" %s\n", type);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* map config resource */
>> +     pcie->config_axi_slave_base =
>> +             map_resource_base(pcie, "config_axi_slave");
>> +     if (IS_ERR(pcie->config_axi_slave_base))
>> +             return PTR_ERR(pcie->config_axi_slave_base);
>> +
>> +     /* map csr resource */
>> +     pcie->csr_axi_slave_base = map_resource_base(pcie, "csr_axi_slave");
>> +     if (IS_ERR(pcie->csr_axi_slave_base))
>> +             return PTR_ERR(pcie->csr_axi_slave_base);
>> +
>> +     /* map gpio resource */
>> +     pcie->gpio_slave_base = remap_reg_base(pcie, "gpio_slave");
>> +     if (IS_ERR(pcie->gpio_slave_base))
>> +             return PTR_ERR(pcie->gpio_slave_base);
>> +
>> +     /* map gpio resource */
>> +     pcie->apb_csr_base = remap_reg_base(pcie, "apb_csr");
>> +     if (IS_ERR(pcie->apb_csr_base))
>> +             return PTR_ERR(pcie->gpio_slave_base);
>> +
>> +     /* map IRQ resource */
>> +     pcie->irq = pcie_request_irq(pcie);
>> +     if (pcie->irq  < 0)
>> +             return pcie->irq;
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + *   access_paged_register - routine to access paged register of root complex
>> + *
>> + *   @pcie   : pointer to rootport
>> + *   @write  : type of operation flag
>> + *   @val    : value to be written to the register
>> + *   @offset : full offset of the register address
>> + *
>> + *   registers of RC are paged, with pg_sel field of the PAB_CTRL_REG
>> + *   register needs to be updated with page of the register, before
>> + *   accessing least significant 10 bits offset
>> + *
>> + *   This routine does the PAB_CTRL_REG updation and split access of the
>> + *   offset
>> + *
>> + *
>
> Remove the above two unnecessary blank lines.
>
>> + */
>> +u32 access_paged_register(struct mobiveil_pcie *pcie,
>
> Must be static.
>
>> +                                u32 write, u32 val,
>> +                                u32 offset)
>
> Fill lines with parameters.  These would fit on two lines instead of
> three.
>
>> +{
>> +     union pab_ctrl_t pab_ctrl;
>> +     u32 off = (offset & 0x3FF) + 0xC00;
>> +
>> +     pab_ctrl.dw = csr_readl(pcie, PAB_CTRL_REG);
>> +     pab_ctrl.pg_sel = (offset >> 10) & 0x3F;
>> +     csr_writel(pcie, pab_ctrl.dw, PAB_CTRL_REG);
>> +
>> +     if (write == OP_WRITE)
>> +             csr_writel(pcie, val, off);
>> +     else
>> +             return csr_readl(pcie, off);
>> +     return 0;
>
> This function should be void if there's no possibility of a failure
> return.
>
>> +}
>> +
>> +/*
>> + *   program_ib_windows - routine to program the inbound windows of
>> + *   Rootport
>> + *
>> + *   @pcie   : pointer to rootpor
>> + */
>> +void program_ib_windows(struct mobiveil_pcie *pcie)
>
> Must be static.
>
>> +{
>> +     int value;
>> +     int window = WIN_NUM_1;
>> +     union pab_pex_amap_ctrl_t amap_ctrl;
>> +     int ib_start = 0;
>> +     int size_kb = IB_WIN_SIZE_KB;
>> +
>> +     u64 size64 = (-1 << (AXI_WINDOW_SIZE_BIT + ilog2_u32(size_kb)));
>> +
>> +     value = csr_readl(pcie, PAB_PEX_PIO_CTRL_REG);
>> +     csr_writel(pcie, value | 0x01, PAB_PEX_PIO_CTRL_REG);
>> +
>> +     amap_ctrl.dw =
>> +         access_paged_register(pcie, OP_READ, 0, PAB_PEX_AMAP_CTRL(window));
>> +     amap_ctrl.enable = 1;
>> +     amap_ctrl.type = 2;     /* 0: interrupt, 2: prefetchable memory */
>> +     access_paged_register(pcie, OP_WRITE,
>> +                           amap_ctrl.dw | (size64 & 0xFFFFFFFF),
>> +                           PAB_PEX_AMAP_CTRL(window));
>> +     access_paged_register(pcie, OP_WRITE, (size64 >> 32),
>> +                           PAB_EXT_PEX_AMAP_SIZEN(window));
>> +
>> +     access_paged_register(pcie, OP_WRITE, ib_start,
>> +                           PAB_PEX_AMAP_AXI_WIN(window));
>> +     access_paged_register(pcie, OP_WRITE, ib_start,
>> +                           PAB_PEX_AMAP_PEX_WIN_L(window));
>> +     access_paged_register(pcie, OP_WRITE, 0x00000000,
>> +                           PAB_PEX_AMAP_PEX_WIN_H(window));
>> +}
>> +
>> +/*
>> + *   program_ob_windows - routine to program the outbound windows of R
>
> What's R?
>
>> + *
>> + *   @pcie                 : pointer to rootport
>> + *   @win_num              : window number
>> + *   @pci_axi_window_base  : AXI window base
>> + *   @pex_addr_base_lower  : PCI window base
>> + *   @config_io_bit        : flag bit to indecate memory or IO type
>> + *   @size_kb              : window size requester
>> + */
>> +void program_ob_windows(struct mobiveil_pcie *pcie, int win_num,
>> +                     u64 pci_axi_window_base, u64 pex_addr_base_lower,
>> +                     int config_io_bit, int size_kb)
>> +{
>> +     u32 value, type;
>> +     u64 size64 = (-1 << (AXI_WINDOW_SIZE_BIT + ilog2_u32(size_kb)));
>> +
>> +     /* Program Enable Bit to 1, Type Bit to (00) base 2, AXI Window Size Bit
>> +      * to 4 KB in PAB_AXI_AMAP_CTRL Register
>> +      */
>
> Multi-line comment formatting style is
>
>   /*
>    * Program Enable Bit ...
>    * to 4 KB ...
>    */
>> +     type = config_io_bit;
>> +     value = csr_readl(pcie, PAB_AXI_AMAP_CTRL_REG(win_num));
>> +     csr_writel(pcie,
>> +                1 << ENABLE_BIT | type << TYPE_BIT | (size64 & 0xFFFFFFFF),
>> +                PAB_AXI_AMAP_CTRL_REG(win_num));
>> +     access_paged_register(pcie, OP_WRITE, (size64 >> 32),
>> +                           PAB_EXT_AXI_AMAP_SIZE(win_num));
>> +
>> +     /* Program AXI window base with appropriate value in
>> +      * PAB_AXI_AMAP_AXI_WIN0
>> +      * Register
>> +      */
>
> Comment formatting style.
>
>> +     value = csr_readl(pcie, PAB_AXI_AMAP_AXI_WIN_REG(win_num));
>> +     csr_writel(pcie,
>> +                pci_axi_window_base >> AXI_WINDOW_BASE_BIT <<
>> +                AXI_WINDOW_BASE_BIT, PAB_AXI_AMAP_AXI_WIN_REG(win_num));
>> +
>> +     value = csr_readl(pcie, PAB_AXI_AMAP_PEX_WIN_H_REG(win_num));
>> +     csr_writel(pcie, pex_addr_base_lower,
>> +                PAB_AXI_AMAP_PEX_WIN_L_REG(win_num));
>> +     csr_writel(pcie, 0, PAB_AXI_AMAP_PEX_WIN_H_REG(win_num));
>> +
>
> Remove unnecessary blank line.
>
>> +}
>> +
>> +/*
>> + *   gpio_read -  routine to read a GPIO register
>> + *
>> + *   @pcie - pcie root port
>> + *   @addr - register address
>> + *
>> + */
>
> Unnecessary function comment.
>
>> +int gpio_read(struct mobiveil_pcie *pcie, int addr)
>
> Must be static.
>
>> +{
>> +     return ioread32(pcie->gpio_slave_base + addr);
>> +}
>> +
>> +/*
>> + *   gpio_write -  routine to write a GPIO register
>> + *
>> + *   @pcie - pcie root port
>> + *   @addr - register address
>> + *
>> + */
>> +void gpio_write(struct mobiveil_pcie *pcie, int val, int addr)
>
> Static.
>
>> +{
>> +     iowrite32(val, pcie->gpio_slave_base + addr);
>> +     if (val != gpio_read(pcie, addr)) {
>> +             pr_info("ERROR:gpio_write: expected : %x at: %x, found: %x\n ",
>> +                    val, addr, gpio_read(pcie, addr));
>
> Must use dev_info() (or dev_err()).
>
>> +     }
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_powerup_slot - routine to prepare the RC for config access
>> + *
>> + *   @pcie                 : pointer to rootport
>> + */
>> +void mobiveil_pcie_powerup_slot(struct mobiveil_pcie *pcie)
>
> Static.
>
>> +{
>> +
>> +     int secs = 0;
>
> Unnecessary initialization.
>
>> +
>> +     // sent PRESET to the slot
>> +     gpio_write(pcie, 0x80000000, 0xa0344);
>> +     gpio_write(pcie, 0x80000000, 0xa0348);
>> +     gpio_write(pcie, 0x00000000, 0xa0054);
>> +     gpio_write(pcie, 0x80000000, 0xa0054);
>> +     secs = 4;
>> +     pr_info("mobiveil_pcie_powerup_slot: waitring for %d seconds\n", secs);
>
> Use dev_info().
> s/waitring/waiting/
>
>> +     mdelay(secs * 1000);
>
> Seems excessive.  Is there no way you can check for powerup
> completion?
>
>> +
>
> Remove blank line.
>
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_setup_csr_for_config_access - routine to prepare the RC
>> + *   for config access
>> + *
>> + *   @pcie                 : pointer to rootport
>> + *
>> + */
>
> Unnecessary comment.  It basically says what the function name already
> tells us.  (Plus it's the same comment as for the previous function.)
>
>> +void mobiveil_pcie_setup_csr_for_config_access(struct mobiveil_pcie *pcie)
>
> Static.
>
>> +{
>> +     u32 value;
>> +     union pab_ctrl_t pab_ctrl;
>> +
>> +     /* Program Bus Master Enable Bit in Command Register in PAB Config
>> +      * Space
>> +      */
>
> Comment formatting style.
>
>> +     value = csr_readl(pcie, PCI_COMMAND);
>> +     csr_writel(pcie,
>> +                value | PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
>> +                PCI_COMMAND_MASTER, PCI_COMMAND);
>> +
>> +     /* Program PIO Enable Bit to 1 (and PEX PIO Enable to 1) in PAB_CTRL
>> +      * Register
>> +      */
>
> Style.
>
>> +     pab_ctrl.dw = csr_readl(pcie, PAB_CTRL_REG);
>> +     pab_ctrl.amba_pio = 1;
>> +     pab_ctrl.pex_pio = 1;
>> +     csr_writel(pcie, pab_ctrl.dw, PAB_CTRL_REG);
>> +
>> +     csr_writel(pcie, (PAB_INTP_INTX_MASK | PAB_INTP_MSI_MASK),
>> +                PAB_INTP_AMBA_MISC_ENB);
>> +
>> +     /* Program PIO Enable Bit to 1 and Config Window Enable Bit to 1 in
>> +      * PAB_AXI_PIO_CTRL Register
>> +      */
>
> Style.
>
>> +     value = csr_readl(pcie, PAB_AXI_PIO_CTRL_REG(0));
>> +     csr_writel(pcie, value | 0xf, PAB_AXI_PIO_CTRL_REG(0));
>> +
>> +     /* Config outbound translation window */
>> +     program_ob_windows(pcie,
>> +                        WIN_NUM_0, WIN_1_BASE,
>> +                        0, CFG_WINDOW_TYPE, OB_CFG_WIN_SIZE_KB);
>> +
>> +     /* Memory outbound translation window */
>> +     program_ob_windows(pcie,
>> +                        WIN_NUM_1, WIN_1_BASE + WIN1_BAR_MAP_DISTANCE,
>> +                        BAR_MAP_BASE, MEM_WINDOW_TYPE, OB_MEM_WIN_SIZE_KB);
>> +
>> +     /* Memory inbound  translation window */
>
> s/  / /
>
>> +     program_ib_windows(pcie);
>> +
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_intx_map - routine to setup the INTx related data
>> + *   structures
>> + *
>> + *   @domain   : pointer to IRQ domain
>> + *   @irq      : IRQ number
>> + *   @hwirq    : hardware IRQ number
>> + *
>> + */
>
> Unnecessary function comment.
>
>> +static int mobiveil_pcie_intx_map(struct irq_domain *domain, u32 irq,
>> +                               irq_hw_number_t hwirq)
>> +{
>> +     irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
>> +     irq_set_chip_data(irq, domain->host_data);
>> +     return 0;
>> +}
>> +
>> +/*   INTx domain opeartions structure */
>
> Unnecessary comment, or
> s/opeartions/operations/
>
>> +static const struct irq_domain_ops intx_domain_ops = {
>> +     .map = mobiveil_pcie_intx_map,
>> +};
>> +
>> +/*
>> + *   mobiveil_pcie_destroy_msi - Free MSI number
>> + *   @irq: IRQ to be freed
>> + */
>> +static void mobiveil_pcie_destroy_msi(u32 irq)
>> +{
>> +     struct msi_desc *msi;
>> +     struct mobiveil_pcie *pcie;
>> +
>> +     if (!test_bit(irq, msi_irq_in_use)) {
>> +             msi = irq_get_msi_desc(irq);
>> +             pcie = msi_desc_to_pci_sysdata(msi);
>> +             pr_info("Trying to free unused MSI#%d\n", irq);
>
> dev_err().
>
>> +     } else {
>> +             clear_bit(irq, msi_irq_in_use);
>> +     }
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_assign_msi - Allocate MSI number
>> + *   @pcie: PCIe port structure
>> + *
>> + *   Return: A valid IRQ on success and error value on failure.
>> + */
>> +static int mobiveil_pcie_assign_msi(struct mobiveil_pcie *pcie)
>> +{
>> +     int pos;
>> +
>> +     pos = find_first_zero_bit(msi_irq_in_use, MOBIVEIL_NUM_MSI_IRQS);
>> +     if (pos < MOBIVEIL_NUM_MSI_IRQS)
>> +             set_bit(pos, msi_irq_in_use);
>> +     else
>> +             return -ENOSPC;
>> +
>> +     return pos;
>> +}
>> +
>> +/*
>> + *   mobiveil_msi_teardown_irq - Destroy the MSI
>> + *
>> + *   @chip: MSI Chip descriptor
>> + *   @irq: MSI IRQ to destroy
>> + */
>> +static void mobiveil_msi_teardown_irq(struct msi_controller *chip,
>> +                                   u32 irq)
>> +{
>> +     mobiveil_pcie_destroy_msi(irq);
>
> xilinx does irq_dispose_mapping(irq) here.  Why don't you?
> I don't know what the corresponding setup is; maybe it's because
> xilinx sets it up but you don't?
>
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_msi_setup_irq - routine to compose MSI message descriptor
>> + *
>> + *   @chip   : pointer to MSI controller structure
>> + *   @pdev   : pointer to PCI device
>> + *   @desc   : MSI descriptor to be setup
>> + */
>> +static int mobiveil_pcie_msi_setup_irq(struct msi_controller *chip,
>> +                                    struct pci_dev *pdev,
>> +                                    struct msi_desc *desc)
>> +{
>> +     int hwirq;
>> +     u32 irq;
>> +     struct msi_msg msg;
>> +     phys_addr_t msg_addr;
>> +     struct mobiveil_pcie *pcie = pdev->bus->sysdata;
>> +
>> +     hwirq = mobiveil_pcie_assign_msi(pcie);
>> +     if (hwirq < 0)
>> +             return -EINVAL;
>> +
>> +     irq = irq_create_mapping(pcie->irq_domain, hwirq);
>> +     if (!irq)
>> +             return -EINVAL;
>> +
>> +     irq_set_msi_desc(irq, desc);
>> +
>> +     msg_addr =
>> +         virt_to_phys((void *)pcie->msi_pages + (hwirq * sizeof(int)));
>> +
>> +     msg.address_hi = 0xFFFFFFFF & (msg_addr >> 32);
>> +     msg.address_lo = 0xFFFFFFFF & msg_addr;
>
> Use upper_32_bits() and lower_32_bits().
>
>> +     msg.data = irq;
>> +
>> +     pci_write_msi_msg(irq, &msg);
>> +     max_msi_allocated++;
>
> max_msi_allocated is incremented but never used otherwise, so you can
> remove it.
>
>> +
>> +     return 0;
>> +}
>> +
>> +/* MSI Chip Descriptor */
>
> Unnecessary comment.
>
>> +static struct msi_controller mobiveil_pcie_msi_chip = {
>> +     .setup_irq = mobiveil_pcie_msi_setup_irq,
>> +     .teardown_irq = mobiveil_msi_teardown_irq,
>> +};
>> +
>> +/* HW Interrupt Chip Descriptor */
>
> Unnecessary comment.
>
>> +static struct irq_chip mobiveil_msi_irq_chip = {
>> +     .name = "Mobiveil PCIe MSI",
>> +     .irq_enable = pci_msi_unmask_irq,
>> +     .irq_disable = pci_msi_mask_irq,
>> +     .irq_mask = pci_msi_mask_irq,
>> +     .irq_unmask = pci_msi_unmask_irq,
>> +};
>> +
>> +/*
>> + *   mobiveil_pcie_msi_map - routine to initialize MSI data structures
>> + *
>> + *   @domain :   pointer IRQ domain
>> + *   @irq    :   IRQ number
>> + *   @hwirq  :   hardware IRQ number
>> + */
>
> Unnecessary comment.
>
>> +static int mobiveil_pcie_msi_map(struct irq_domain *domain, u32 irq,
>> +                              irq_hw_number_t hwirq)
>> +{
>> +     irq_set_chip_and_handler(irq, &mobiveil_msi_irq_chip,
>> +                              handle_simple_irq);
>> +     irq_set_chip_data(irq, domain->host_data);
>> +
>> +     return 0;
>> +}
>> +
>> +/*   MSI IRQ Domain operations */
>
> Unnecessary comment.
>
>> +static const struct irq_domain_ops msi_domain_ops = {
>> +     .map = mobiveil_pcie_msi_map,
>> +};
>> +
>> +/*
>> + *   mobiveil_pcie_enable_msi - Enable MSI support
>> + *   @pcie: PCIe port information
>> + */
>
> Unnecessary comment.
>
>> +static void mobiveil_pcie_enable_msi(struct mobiveil_pcie *pcie)
>> +{
>> +     phys_addr_t msg_addr;
>> +
>> +     pcie->msi_pages = (void *)__get_free_pages(GFP_DMA, 0);
>
> This looks like the common pattern of the PCIe host intercepting MSI
> writes to this address, so the write never actually gets to system
> memory, so we don't actually need to allocate a page of system memory;
> we only need a little bus address space.  See
> https://lkml.kernel.org/r/20171109181435.GB7629@bhelgaas-glaptop.roam.corp.google.com
>
> We don't have a good solution for that yet, so I don't have a
> suggestion and this is just FYI.
>
>> +     msg_addr = virt_to_phys((void *)pcie->msi_pages);
>> +     pcie->msi_pages_phys = (void *)msg_addr;
>> +
>> +     writel_relaxed(msg_addr & 0xFFFFFFFF,
>> +                    pcie->apb_csr_base + MSI_BASE_LO_OFFSET);
>> +     writel_relaxed(msg_addr >> 32,
>> +                     pcie->apb_csr_base + MSI_BASE_HI_OFFSET);
>
> Use upper_32_bits() and lower_32_bits().
>
>> +     writel_relaxed(4096,
>> +                     pcie->apb_csr_base + MSI_SIZE_OFFSET);
>> +     writel_relaxed(1 << MSI_ENABLE_BIT_POS,
>> +                     pcie->apb_csr_base + MSI_ENABLE_OFFSET);
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_init_irq_domain - routine to setup IRQ domains for
>> + *   both INTx and MSI interrupts
>> + *
>> + *   @pcie : pointer to pci root port
>> + */
>
> Unnecessary comment.
>
>> +static int mobiveil_pcie_init_irq_domain(struct mobiveil_pcie *pcie)
>> +{
>> +     struct device *dev = &pcie->pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +
>> +     /* Setup INTx */
>> +     pcie->irq_domain =
>> +         irq_domain_add_linear(node, INTX_NUM + 1, &intx_domain_ops, pcie);
>> +
>> +     if (!pcie->irq_domain) {
>> +             dev_err(dev, "Failed to get a INTx IRQ domain\n");
>> +             return -ENOMEM;
>> +     }
>> +     /* Setup MSI */
>> +     if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +             pcie->irq_domain = irq_domain_add_linear(node,
>> +                                              MOBIVEIL_NUM_MSI_IRQS,
>> +                                              &msi_domain_ops,
>> +                                              &mobiveil_pcie_msi_chip);
>> +             if (!pcie->irq_domain) {
>> +                     dev_err(dev, "Failed to get a MSI IRQ domain\n");
>> +                     return PTR_ERR(pcie->irq_domain);
>> +             }
>> +
>> +             mobiveil_pcie_enable_msi(pcie);
>> +     }
>> +     return 0;
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_free_irq_domain - routine to free the IRQ domain of
>> + *   the root port
>> + *
>> + *   @pcie : pointer to pci root port
>> + */
>> +static void mobiveil_pcie_free_irq_domain(struct mobiveil_pcie *pcie)
>> +{
>> +     int i;
>> +     u32 irq;
>> +
>> +     for (i = 0; i < INTX_NUM; i++) {
>> +             irq = irq_find_mapping(pcie->irq_domain, i + 1);
>> +             if (irq > 0)
>> +                     irq_dispose_mapping(irq);
>> +     }
>> +     if (pcie->irq_domain)
>> +             irq_domain_remove(pcie->irq_domain);
>> +
>
> Unnecessary blank line.
>
> I can't match this function up with similar code in other drivers.
>
> irq_dispose_mapping() is called by:
>
>   tegra_msi_teardown_irq()
>   tegra_pcie_disable_msi()
>   iproc_msi_exit()
>   mtk_msi_teardown_irq()
>   xilinx_msi_teardown_irq()
>
> tegra_pcie_disable_msi() and iproc_msi_exit are the most similar.
> They're the only ones that call it in a loop, but they loop over MSI
> IRQs, and you're looping over INTx IRQs.
>
> So I don't know if other drivers are missing something, or this is
> something unnecessary in *this* driver.
>
> irq_domain_remove() is called by:
>
>   advk_pcie_remove_msi_irq_domain()
>   advk_pcie_remove_irq_domain()
>   tegra_pcie_disable_msi()
>   xgene_free_domains()
>   altera_free_domains()
>   iproc_msi_free_domains()
>   rockchip_pcie_remove()
>
> You call this from mobiveil_pcie_remove(), so I guess you could just
> call irq_domain_remove() directly from there.  And you shouldn't need
> to check it for NULL because the probe fails if we can't allocation
> pcie->irq_domain, so we should never get there if it's NULL.
>
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_probe  - probe routine which will get called by kernel
>> + *   once the RC is discovered
>> + *
>> + *   @pdev :  pointer to platform device
>> + */
>
> Unnecessary comment.
>
>> +static int mobiveil_pcie_probe(struct platform_device *pdev)
>> +{
>> +     struct mobiveil_pcie *pcie;
>> +     struct pci_bus *bus;
>> +     struct pci_bus *child;
>> +     int ret, reset_cnt = 0;
>> +     struct device_node *np = pdev->dev.of_node;
>> +
>> +     resource_size_t iobase = 0;
>> +     LIST_HEAD(res);
>> +
>> +     /* allocate the PCIe port */
>> +     pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
>> +     if (!pcie)
>> +             return -ENOMEM;
>> +
>> +     pcie->pdev = pdev;
>> +
>> +     /* parse the device tree */
>> +     ret = mobiveil_pcie_parse_dt(pcie);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Parsing DT failed, ret: %x\n", ret);
>
> Use a local "struct device *dev" variable to avoid repeating
> "&pdev->dev".
>
>> +             return ret;
>> +     }
>> +
>> +     if (!mobiveil_pcie_link_is_up(pcie)) {
>> +             /* if FSBL is not patched, link wont be up so lets bring it
>> +              * up by writng DIRM and OEN registers EMIO 6:0 programing
>> +              * sequence [3:0] = Link Width; [6:4] = link Speed. Current
>> +              * setting width=4, speed = 1
>
> s/wont/won't/
> s/lets/let's/
> s/writng/writing/
> s/programing/programming/
> s/link Speed/Link Speed/
>
>> +              */
>
> Comment style.
>
>> +             gpio_write(pcie, 0x7f, 0xa02c4);
>> +             gpio_write(pcie, 0x7f, 0xa02c8);
>> +             gpio_write(pcie, 0x14, 0xa004c);
>> +
>> +             gpio_write(pcie, 0x0200, 0xa0244);
>> +             gpio_write(pcie, 0x0200, 0xa0248);
>> +             gpio_write(pcie, 0x37f7, 0x180208);
>> +             gpio_write(pcie, 0xfdff, 0xa0044);
>> +
>> +             pr_info("waiting for %d seconds\n", 2);
>
> dev_info().  If it's a constant, no point in using %d.
>
>> +             mdelay(2 * 1000);
>> +             mobiveil_pcie_powerup_slot(pcie);
>> +
>> +             while (!mobiveil_pcie_link_is_up(pcie)) {
>> +                     pr_info("%s: PRESET retry, reset_cnt : %d\n",
>> +                          __func__, reset_cnt++);
>
> dev_info().
>
>> +                     mobiveil_pcie_powerup_slot(pcie);
>> +             }
>> +
>> +     }
>
> This looks a little like tegra_pcie_enable_controller() or the
> dra7xx_pcie_establish_link(), exynos_pcie_establish_link(), etc.,
> functions.  Please factor this out.  I think the .*_pcie_host_init()
> and .*_pcie_establish_link() pattern is a good one to follow.
>
>> +
>> +     pr_info("%s: PCIE link is up , resets: %x, state: %x\n",
>> +             __func__,
>> +             reset_cnt,
>> +             csr_readl(pcie, LTSSM_STATE_STATUS_REG)
>> +                             & LTSSM_STATUS_CODE_MASK);
>
> dev_info().
>
>> +
>> +     INIT_LIST_HEAD(&pcie->resources);
>> +
>> +     /* configure all inbound and outbound windows and prepare the RC for
>> +      * config access
>> +      */
>> +     mobiveil_pcie_setup_csr_for_config_access(pcie);
>> +
>> +     /* fixup for PCIe config space register data */
>> +     csr_writel(pcie, 0x060402AB, PAB_INTP_AXI_PIO_CLASS_REG);
>> +
>> +     /* parse the host bridge base addresses from the device tree file */
>> +     ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &iobase);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Getting bridge resources failed\n");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     /* initialize the IRQ domains */
>> +     ret = mobiveil_pcie_init_irq_domain(pcie);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
>> +             return ret;
>> +     }
>> +
>> +     /* create the PCIe root bus */
>> +     bus =
>> +         pci_create_root_bus(&pdev->dev, 0, &mobiveil_pcie_ops, pcie, &res);
>> +     if (!bus)
>> +             return -ENOMEM;
>> +
>> +     /* setup MSI, if enabled */
>> +     if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +             mobiveil_pcie_msi_chip.dev = &pcie->pdev->dev;
>> +             bus->msi = &mobiveil_pcie_msi_chip;
>> +     }
>> +
>> +     /* setup the kernel resources for the newly added PCIe root bus */
>> +     pci_scan_child_bus(bus);
>
> Use pci_scan_root_bus_bridge().  For example, see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=123db533072e
>
>> +     pci_assign_unassigned_bus_resources(bus);
>> +
>> +     list_for_each_entry(child, &bus->children, node)
>> +             pcie_bus_configure_settings(child);
>> +
>> +     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>
> pci_scan_root_bus_bridge() also takes care of this pci_fixup_irqs()
> (which doesn't exist anymore); see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6ab380957838
>
>> +     pci_bus_add_devices(bus);
>> +     platform_set_drvdata(pdev, pcie);
>> +
>> +     pr_info("%s: platform driver registered\n", __func__);
>
> Remove.
>
>> +     return ret;
>> +}
>> +
>> +/*
>> + *   mobiveil_pcie_remove  - routine which will cleanup the driver removal
>> + *
>> + *   @pdev :  pointer to platform device
>> + */
>> +
>
> Unnecessary comment.
>
>> +static int mobiveil_pcie_remove(struct platform_device *pdev)
>> +{
>> +     struct mobiveil_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +     mobiveil_pcie_free_irq_domain(pcie);
>> +     platform_set_drvdata(pdev, NULL);
>
> Unnecessary.
>
>> +     pr_info("%s: platform driver unregistered\n", __func__);
>
> Unnecessary pr_info(); remove.
>
>> +     return 0;
>> +}
>> +
>> +/*   device id structure mentioning the compatible string to search for
>> + *   in the device tree
>> + */
>
> Unnecessary comment.
>
>> +static const struct of_device_id mobiveil_pcie_of_match[] = {
>> +     {.compatible = "mbvl,axi-pcie-host-1.00",},
>> +     {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, mobiveil_pcie_of_match);
>> +
>> +/*   platform driver structure */
>
> Unnecessary comment.
>
>> +static struct platform_driver mobiveil_pcie_driver = {
>> +     .probe = mobiveil_pcie_probe,
>> +     .remove = mobiveil_pcie_remove,
>> +     .driver = {
>> +                .name = "mobiveil-pcie",
>> +                .of_match_table = mobiveil_pcie_of_match,
>> +                .suppress_bind_attrs = true,
>> +                },
>> +};
>> +
>> +/*   Declare the platform driver */
>
> Unnecessary comment.
>
>> +builtin_platform_driver(mobiveil_pcie_driver);
>> +
>> +/*   kernel module descriptions */
>
> Unnecessary comment.
>
>> +MODULE_LICENSE("GPL");
>
> The header comment claims "GPL v2", but this says just "GPL".  They
> should match.
>
>> +MODULE_DESCRIPTION("Mobiveil PCIe host controller driver");
>> +MODULE_AUTHOR("Subrahmanya Lingappa <l.subrahmanya@mobiveil.co.in>");
>> --
>> 1.8.3.1
>>
>>