| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eaa5dd6b-e9bb-42a1-d249-b4ee1d97613@linux.intel.com>
Date: Fri, 17 Jan 2025 11:09:23 -0800 (PST)
From: matthew.gerlach@...ux.intel.com
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
cc: lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org,
dinguyen@...nel.org, joyce.ooi@...el.com, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
matthew.gerlach@...era.com,
"D M, Sharath Kumar" <sharath.kumar.d.m@...el.com>,
D@...nkpad.smtp.subspace.kernel.org,
M@...nkpad.smtp.subspace.kernel.org
Subject: Re: [PATCH v3 5/5] PCI: altera: Add Agilex support
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@...el.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@...el.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.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.
>
>> +
>> 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
>
> --
> மணிவண்ணன் சதாசிவம்
>
Powered by blists - more mailing lists