Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 02 Apr 2021 12:17:09 +0100
Message-ID: <87a6qhey16.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Kishon Vijay Abraham I <kishon@ti.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Tom Joseph <tjoseph@cadence.com>, <linux-omap@vger.kernel.org>,
        <linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        Lokesh Vutla <lokeshvutla@ti.com>
Subject: Re: [PATCH 3/4] PCI: j721e: Add PCIe support for j7200
In-Reply-To: <20210325090936.9306-4-kishon@ti.com>
References: <20210325090936.9306-1-kishon@ti.com>
        <20210325090936.9306-4-kishon@ti.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 25 Mar 2021 09:09:35 +0000,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> J7200 has the same PCIe IP as in J721E with minor changes in the
> wrapper. Add PCIe support for j7200 accounting for the wrapper
> changes in pci-j721e.c
> Changes from J721E:
>  *) Allows byte access of bridge configuration space registers
>  *) Changes in legacy interrupt register map
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 111 ++++++++++++++++++---
>  1 file changed, 99 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 17db86a51ca8..f175f116abf6 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -27,6 +27,7 @@
>  #define STATUS_REG_SYS_2	0x508
>  #define STATUS_CLR_REG_SYS_2	0x708
>  #define LINK_DOWN		BIT(1)
> +#define J7200_LINK_DOWN		BIT(10)
>  
>  #define EOI_REG			0x10
>  
> @@ -35,6 +36,10 @@
>  #define STATUS_CLR_REG_SYS_0	0x700
>  #define INTx_EN(num)		(1 << (num))
>  
> +#define ENABLE_REG_SYS_1	0x104
> +#define STATUS_REG_SYS_1	0x504
> +#define SYS1_INTx_EN(num)	(1 << (22 + (num)))
> +
>  #define J721E_PCIE_USER_CMD_STATUS	0x4
>  #define LINK_TRAINING_ENABLE		BIT(0)
>  
> @@ -48,6 +53,14 @@ enum link_status {
>  	LINK_UP_DL_COMPLETED,
>  };
>  
> +#define USER_EOI_REG		0xC8
> +enum eoi_reg {
> +	EOI_DOWNSTREAM_INTERRUPT,
> +	EOI_FLR_INTERRUPT,
> +	EOI_LEGACY_INTERRUPT,
> +	EOI_POWER_STATE_INTERRUPT,
> +};
> +
>  #define J721E_MODE_RC			BIT(7)
>  #define LANE_COUNT_MASK			BIT(8)
>  #define LANE_COUNT(n)			((n) << 8)
> @@ -65,6 +78,8 @@ struct j721e_pcie {
>  	void __iomem		*user_cfg_base;
>  	void __iomem		*intd_cfg_base;
>  	struct irq_domain	*legacy_irq_domain;
> +	bool			is_intc_v1;
> +	u32			link_irq_reg_field;
>  };
>  
>  enum j721e_pcie_mode {
> @@ -75,6 +90,9 @@ enum j721e_pcie_mode {
>  struct j721e_pcie_data {
>  	enum j721e_pcie_mode	mode;
>  	bool quirk_retrain_flag;
> +	bool			is_intc_v1;
> +	bool			byte_access_allowed;
> +	const struct cdns_pcie_ops *ops;
>  };
>  
>  static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
> @@ -106,12 +124,12 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
>  	u32 reg;
>  
>  	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2);
> -	if (!(reg & LINK_DOWN))
> +	if (!(reg & pcie->link_irq_reg_field))
>  		return IRQ_NONE;
>  
>  	dev_err(dev, "LINK DOWN!\n");
>  
> -	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, LINK_DOWN);
> +	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, pcie->link_irq_reg_field);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -119,12 +137,40 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
>  {
>  	u32 reg;
>  
> +	pcie->link_irq_reg_field = J7200_LINK_DOWN;
> +	if (pcie->is_intc_v1)
> +		pcie->link_irq_reg_field = LINK_DOWN;
> +
>  	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_2);
> -	reg |= LINK_DOWN;
> +	reg |= pcie->link_irq_reg_field;
>  	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
>  }
>  
>  static void j721e_pcie_legacy_irq_handler(struct irq_desc *desc)
> +{
> +	struct j721e_pcie *pcie = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	int virq;
> +	u32 reg;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_1);
> +		if (!(reg & SYS1_INTx_EN(i)))
> +			continue;
> +
> +		virq = irq_find_mapping(pcie->legacy_irq_domain, i);
> +		generic_handle_irq(virq);
> +		j721e_pcie_user_writel(pcie, USER_EOI_REG,
> +				       EOI_LEGACY_INTERRUPT);

Exact same comment as in the previous patch: this EOI (which I assume
is used to regenerate the GIC edge at after handling the INTx level
interrupt) must be placed in a irq_eoi() callback.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.