Re: [PATCH 3/4] PCI: j721e: Add PCIe support for j7200

[Marc Zyngier <maz@kernel.org>]

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.