| 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: <e3d0e811-cc52-ae92-38b0-4082b03c80c9@linux.intel.com>
Date: Fri, 17 Jan 2025 13: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>
Subject: Re: [PATCH v3 5/5] PCI: altera: Add Agilex support
On Fri, 17 Jan 2025, matthew.gerlach@...ux.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@...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.
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
>
>>
>> --
>> மணிவண்ணன் சதாசிவம்
>
Powered by blists - more mailing lists