Re: [PATCH v3 5/5] PCI: altera: Add Agilex support

[matthew.gerlach@linux.intel.com]

[-- Attachment #1: Type: text/plain, Size: 11163 bytes --]



On Fri, 17 Jan 2025, matthew.gerlach@linux.intel.com wrote:

>
>
> On Thu, 16 Jan 2025, Manivannan Sadhasivam wrote:
>
>> On Wed, Jan 08, 2025 at 10:59:09AM -0600, Matthew Gerlach wrote:
>>> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
>>> 
>>> Add PCIe root port controller support Agilex family of chips.
>>> 
>> 
>> Please add more info about the PCIe controller in description. Like speed,
>> lanes, IP revision etc... Also, you are introducing ep_{read/write}_cfg()
>> callbacks, so they should also be described here.
>
> I will add more info about the Agilex PCIe controller and describe 
> ep_{read/write}_cfg() callbacks.
>
>> 
>>> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>> v3:
>>>  - Remove accepted patches from patch set.
>>> 
>>> v2:
>>>  - Match historical style of subject.
>>>  - Remove unrelated changes.
>>>  - Fix indentation.
>>> ---
>>>  drivers/pci/controller/pcie-altera.c | 246 ++++++++++++++++++++++++++-
>>>  1 file changed, 237 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/drivers/pci/controller/pcie-altera.c 
>>> b/drivers/pci/controller/pcie-altera.c
>>> index eb55a7f8573a..da4ae21d661d 100644
>>> --- a/drivers/pci/controller/pcie-altera.c
>>> +++ b/drivers/pci/controller/pcie-altera.c
>>> @@ -77,9 +77,19 @@
>>>  #define S10_TLP_FMTTYPE_CFGWR0		0x45
>>>  #define S10_TLP_FMTTYPE_CFGWR1		0x44
>>> 
>>> +#define AGLX_RP_CFG_ADDR(pcie, reg)	(((pcie)->hip_base) + (reg))
>>> +#define AGLX_RP_SECONDARY(pcie)		\
>>> +	readb(AGLX_RP_CFG_ADDR(pcie, PCI_SECONDARY_BUS))
>>> +
>>> +#define AGLX_BDF_REG			0x00002004
>>> +#define AGLX_ROOT_PORT_IRQ_STATUS	0x14c
>>> +#define AGLX_ROOT_PORT_IRQ_ENABLE	0x150
>>> +#define CFG_AER				BIT(4)
>>> +
>>>  enum altera_pcie_version {
>>>  	ALTERA_PCIE_V1 = 0,
>>>  	ALTERA_PCIE_V2,
>>> +	ALTERA_PCIE_V3,
>>>  };
>>>
>>>  struct altera_pcie {
>>> @@ -102,6 +112,11 @@ struct altera_pcie_ops {
>>>  			   int size, u32 *value);
>>>  	int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
>>>  			    int where, int size, u32 value);
>>> +	int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
>>> +			   unsigned int devfn, int where, int size, u32 
>>> *value);
>>> +	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
>>> +			    unsigned int devfn, int where, int size, u32 
>>> value);
>>> +	void (*rp_isr)(struct irq_desc *desc);
>>>  };
>>>
>>>  struct altera_pcie_data {
>>> @@ -112,6 +127,9 @@ struct altera_pcie_data {
>>>  	u32 cfgrd1;
>>>  	u32 cfgwr0;
>>>  	u32 cfgwr1;
>>> +	u32 port_conf_offset;
>>> +	u32 port_irq_status_offset;
>>> +	u32 port_irq_enable_offset;
>>>  };
>>>
>>>  struct tlp_rp_regpair_t {
>>> @@ -131,6 +149,28 @@ static inline u32 cra_readl(struct altera_pcie *pcie, 
>>> const u32 reg)
>>>  	return readl_relaxed(pcie->cra_base + reg);
>>>  }
>>> 
>>> +static inline void cra_writew(struct altera_pcie *pcie, const u32 value,
>>> +			      const u32 reg)
>> 
>> No need to add inline keyword to .c files. Compiler will inline the 
>> function
>> anyway if needed.
>
> Using inline is consistent with existing cra_writel and cra_readl.
>
>> 
>>> +{
>>> +	writew_relaxed(value, pcie->cra_base + reg);
>>> +}
>>> +
>>> +static inline u32 cra_readw(struct altera_pcie *pcie, const u32 reg)
>>> +{
>>> +	return readw_relaxed(pcie->cra_base + reg);
>>> +}
>>> +
>>> +static inline void cra_writeb(struct altera_pcie *pcie, const u32 value,
>>> +			      const u32 reg)
>>> +{
>>> +	writeb_relaxed(value, pcie->cra_base + reg);
>>> +}
>>> +
>>> +static inline u32 cra_readb(struct altera_pcie *pcie, const u32 reg)
>>> +{
>>> +	return readb_relaxed(pcie->cra_base + reg);
>>> +}
>>> +
>>>  static bool altera_pcie_link_up(struct altera_pcie *pcie)
>>>  {
>>>  	return !!((cra_readl(pcie, RP_LTSSM) & RP_LTSSM_MASK) == LTSSM_L0);
>>> @@ -145,6 +185,15 @@ static bool s10_altera_pcie_link_up(struct 
>>> altera_pcie *pcie)
>>>  	return !!(readw(addr) & PCI_EXP_LNKSTA_DLLLA);
>>>  }
>>> 
>>> +static bool aglx_altera_pcie_link_up(struct altera_pcie *pcie)
>>> +{
>>> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie,
>>> +				   pcie->pcie_data->cap_offset +
>>> +				   PCI_EXP_LNKSTA);
>>> +
>>> +	return !!(readw(addr) & PCI_EXP_LNKSTA_DLLLA);
>> 
>> Why this can't be readw_relaxed()?
>
> This can be changed to readw_relaxed().
>
>> 
>>> +}
>>> +
>>>  /*
>>>   * Altera PCIe port uses BAR0 of RC's configuration space as the 
>>> translation
>>>   * from PCI bus to native BUS.  Entire DDR region is mapped into PCIe 
>>> space
>>> @@ -425,6 +474,103 @@ static int s10_rp_write_cfg(struct altera_pcie 
>>> *pcie, u8 busno,
>>>  	return PCIBIOS_SUCCESSFUL;
>>>  }
>>> 
>>> +static int aglx_rp_read_cfg(struct altera_pcie *pcie, int where,
>>> +			    int size, u32 *value)
>>> +{
>>> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
>>> +
>>> +	switch (size) {
>>> +	case 1:
>>> +		*value = readb(addr);
>> 
>> Same question as above. Why the relaxed variant is not used here?
>
> Yes, the relaxed variant can be used here too.
>
>> 
>>> +		break;
>>> +	case 2:
>>> +		*value = readw(addr);
>>> +		break;
>>> +	default:
>>> +		*value = readl(addr);
>>> +		break;
>>> +	}
>>> +
>>> +	/* interrupt pin not programmed in hardware, set to INTA */
>>> +	if (where == PCI_INTERRUPT_PIN && size == 1 && !(*value))
>>> +		*value = 0x01;
>>> +	else if (where == PCI_INTERRUPT_LINE && !(*value & 0xff00))
>>> +		*value |= 0x0100;
>>> +
>>> +	return PCIBIOS_SUCCESSFUL;
>>> +}
>>> +
>>> +static int aglx_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
>>> +			     int where, int size, u32 value)
>>> +{
>>> +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
>>> +
>>> +	switch (size) {
>>> +	case 1:
>>> +		writeb(value, addr);
>>> +		break;
>>> +	case 2:
>>> +		writew(value, addr);
>>> +		break;
>>> +	default:
>>> +		writel(value, addr);
>>> +		break;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Monitor changes to PCI_PRIMARY_BUS register on root port
>>> +	 * and update local copy of root bus number accordingly.
>>> +	 */
>>> +	if (busno == pcie->root_bus_nr && where == PCI_PRIMARY_BUS)
>>> +		pcie->root_bus_nr = value & 0xff;
>>> +
>>> +	return PCIBIOS_SUCCESSFUL;
>>> +}
>>> +
>>> +static int aglx_ep_write_cfg(struct altera_pcie *pcie, u8 busno,
>>> +			     unsigned int devfn, int where, int size, u32 
>>> value)
>>> +{
>>> +	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
>>> +	if (busno > AGLX_RP_SECONDARY(pcie))
>>> +		where |= (1 << 12); /* type 1 */
>> 
>> Can you use macro definition for this?
>
> Use a macro like BIT(12)?
>
>> 
>>> +
>>> +	switch (size) {
>>> +	case 1:
>>> +		cra_writeb(pcie, value, where);
>>> +		break;
>>> +	case 2:
>>> +		cra_writew(pcie, value, where);
>>> +		break;
>>> +	default:
>>> +		cra_writel(pcie, value, where);
>>> +			break;
>>> +	}
>>> +
>>> +	return PCIBIOS_SUCCESSFUL;
>>> +}
>>> +
>>> +static int aglx_ep_read_cfg(struct altera_pcie *pcie, u8 busno,
>>> +			    unsigned int devfn, int where, int size, u32 
>>> *value)
>>> +{
>>> +	cra_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
>>> +	if (busno > AGLX_RP_SECONDARY(pcie))
>>> +		where |= (1 << 12); /* type 1 */
>> 
>> Same here.
>> 
>>> +
>>> +	switch (size) {
>>> +	case 1:
>>> +		*value = cra_readb(pcie, where);
>>> +		break;
>>> +	case 2:
>>> +		*value = cra_readw(pcie, where);
>>> +		break;
>>> +	default:
>>> +		*value = cra_readl(pcie, where);
>>> +		break;
>>> +	}
>>> +
>>> +	return PCIBIOS_SUCCESSFUL;
>>> +}
>>> +
>>>  static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
>>>  				 unsigned int devfn, int where, int size,
>>>  				 u32 *value)
>>> @@ -437,6 +583,10 @@ static int _altera_pcie_cfg_read(struct altera_pcie 
>>> *pcie, u8 busno,
>>>  		return pcie->pcie_data->ops->rp_read_cfg(pcie, where,
>>>  							 size, value);
>>> 
>>> +	if (pcie->pcie_data->ops->ep_read_cfg)
>>> +		return pcie->pcie_data->ops->ep_read_cfg(pcie, busno, devfn,
>>> +							where, size, value);
>> 
>> Why do you need to call both rp_read_cfg() and ep_read_cfg()? This looks 
>> wrong.

I missed this comment in my first reply. The functions, rp_read_cfg() and 
ep_read_cfg(), are not both being called. The flow is if, else if, else.

If root port and rp_read_cfg exits
else if eq_read_cfg() exists
else perform original code path.

Matthew
>> 
>>> +
>>>  	switch (size) {
>>>  	case 1:
>>>  		byte_en = 1 << (where & 3);
>>> @@ -481,6 +631,10 @@ static int _altera_pcie_cfg_write(struct altera_pcie 
>>> *pcie, u8 busno,
>>>  		return pcie->pcie_data->ops->rp_write_cfg(pcie, busno,
>>>  						     where, size, value);
>>> 
>>> +	if (pcie->pcie_data->ops->ep_write_cfg)
>>> +		return pcie->pcie_data->ops->ep_write_cfg(pcie, busno, devfn,
>>> +						     where, size, value);
>>> +
>>>  	switch (size) {
>>>  	case 1:
>>>  		data32 = (value & 0xff) << shift;
>>> @@ -659,7 +813,30 @@ static void altera_pcie_isr(struct irq_desc *desc)
>>>  				dev_err_ratelimited(dev, "unexpected IRQ, 
>>> INT%d\n", bit);
>>>  		}
>>>  	}
>>> +	chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> +static void aglx_isr(struct irq_desc *desc)
>>> +{
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	struct altera_pcie *pcie;
>>> +	struct device *dev;
>>> +	u32 status;
>>> +	int ret;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +	pcie = irq_desc_get_handler_data(desc);
>>> +	dev = &pcie->pdev->dev;
>>> 
>>> +	status = readl(pcie->hip_base + pcie->pcie_data->port_conf_offset +
>>> +		       pcie->pcie_data->port_irq_status_offset);
>>> +	if (status & CFG_AER) {
>>> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
>>> +		if (ret)
>>> +			dev_err_ratelimited(dev, "unexpected IRQ\n");
>> 
>> It'd be good to print the IRQ number in error log.
>
> Adding the IRQ number to the error log would be helpful.
>
>> 
>>> +	}
>>> +	writel(CFG_AER, (pcie->hip_base + pcie->pcie_data->port_conf_offset +
>>> +			 pcie->pcie_data->port_irq_status_offset));
>> 
>> You should clear the IRQ before handling it.
>
> Yes, the IRQ should be cleared before handling it.
>
>>
>>>  	chained_irq_exit(chip, desc);
>>>  }
>>> 
>> 
>> [...]
>> 
>>> -	/* clear all interrupts */
>>> -	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
>>> -	/* enable all interrupts */
>>> -	cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
>>> -	altera_pcie_host_init(pcie);
>>> +	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
>>> +	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
>>> +		/* clear all interrupts */
>>> +		cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
>>> +		/* enable all interrupts */
>>> +		cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
>>> +		altera_pcie_host_init(pcie);
>>> +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
>>> +		writel(CFG_AER,
>>> +		       pcie->hip_base + pcie->pcie_data->port_conf_offset +
>>> +		       pcie->pcie_data->port_irq_enable_offset);
>> 
>> Why altera_pcie_host_init() is not called for ALTERA_PCIE_V3?
>
> The V3 hardware does not need to perform a link retraining in order to 
> establish link at higher speeds.
>
>> 
>> - Mani
>
> Thank you for your feedback,
> Matthew Gerlach
>
>> 
>> -- 
>> மணிவண்ணன் சதாசிவம்
>