On Fri, 24 Jan 2025, Ilpo Järvinen wrote:

> On Thu, 23 Jan 2025, Matthew Gerlach wrote:
>
>> From: "D M, Sharath Kumar" <sharath.kumar.d.m@intel.com>
>>
>> Add PCIe root port controller support for the Agilex family of chips.
>> The Agilex PCIe IP has three variants that are mostly sw compatible,
>> except for a couple register offsets. The P-Tile variant supports
>> Gen3/Gen4 1x16. The F-Tile variant supports Gen3/Gen4 4x4, 4x8, and 4x16.
>> The R-Tile variant improves on the F-Tile variant by adding Gen5 support.
>>
>> To simplify the implementation of pci_ops read/write functions,
>> ep_{read/write}_cfg() callbacks were added to struct altera_pci_ops
>> to easily distinguish between hardware variants.
>>
>> Signed-off-by: D M, Sharath Kumar <sharath.kumar.d.m@intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>


[snip]

>> +}
>> +
>> +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_relaxed(addr) & PCI_EXP_LNKSTA_DLLLA);
>
> This returns bool so double negations are not necessary.

I will remove unecessary !!

>
>> +}
>> +
>>  /*
>>   * 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_relaxed(addr);
>> +		break;
>> +	case 2:
>> +		*value = readw_relaxed(addr);
>> +		break;
>> +	default:
>> +		*value = readl_relaxed(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_relaxed(value, addr);
>> +		break;
>> +	case 2:
>> +		writew_relaxed(value, addr);
>> +		break;
>> +	default:
>> +		writel_relaxed(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 |= BIT(12); /* type 1 */
>
> Add a define to replace the comment?

I will create a suitably name macro; so that a comment won't be necessary.

>
>> +
>> +	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 |= BIT(12); /* type 1 */
>
> Same here?

Yes, use a better macro here too.

>
> -- 
> i.

Thanks for the review,
Matthew Gerlach

>
>