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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> 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@xxxxxxxxxx
>>
>> Signed-off-by: Subrahmanya Lingappa <l.subrahmanya@xxxxxxxxxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxx>
>> +L: linux-pci@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxx>
>> L: linux-pci@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
> 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@xxxxxxxxxxxxxx>");
>> --
>> 1.8.3.1
>>
>>
