Thanks for your patch. If/when you revise it, run "git log --oneline
drivers/pci/controller/pcie-altera.c" and match the style of subject
lines, e.g.,
PCI: altera: Add Intel Agilex support
On Fri, Jun 16, 2023 at 12:03:13PM +0530, sharath.kumar.d.m@xxxxxxxxx wrote:
> From: D M Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
Also, please include a commit log.
Probably should also update the Kconfig help text to mention Agilex in
addition to Altera.
> +#include <linux/bitops.h>
I don't think this is needed.
> +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);
> + break;
> + case 2:
> + *value = readw(addr);
> + break;
> + default:
> + *value = readl(addr);
> + break;
> + }
> +
> + if (where == 0x3d)
> + *value = 0x01;
> + if (where == 0x3c)
> + *value |= 0x0100;
This magic needs a comment. Apparently it works around some hardware
defect? What happens if this is a single byte read? Looks like it
may set more than one byte of *value.
> +static int aglx_ep_write_cfg(struct altera_pcie *pcie, u8 busno,
> + unsigned int devfn, int where, int size, u32 value)
> +{
> + cs_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
> + if (busno > AGLX_RP_SECONDARY(pcie)) {
> + /* type 1 */
> + switch (size) {
> + case 1:
> + cs_writeb(pcie, value, ((1 << 12) | where));
> + break;
> + case 2:
> + cs_writew(pcie, value, ((1 << 12) | where));
> + break;
> + default:
> + cs_writel(pcie, value, ((1 << 12) | where));
> + break;
> + }
> + } else {
> + /* type 0 */
> + switch (size) {
> + case 1:
> + cs_writeb(pcie, value, where);
> + break;
> + case 2:
> + cs_writew(pcie, value, where);
> + break;
> + default:
> + cs_writel(pcie, value, where);
> + break;
> + }
> + }
These switches could be combined, e.g.,
if (busno > AGLX_RP_SECONDARY(pcie))
where |= 1 << 12;
> + if (status & CFG_AER) {
> + ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> + if (ret)
> + dev_err_ratelimited(dev, "unexpected IRQ,\n");
Spurious "," at end of line.
> @@ -692,11 +904,27 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
> {
> struct platform_device *pdev = pcie->pdev;
>
> - pcie->cra_base = devm_platform_ioremap_resource_byname(pdev, "Cra");
> - if (IS_ERR(pcie->cra_base))
> - return PTR_ERR(pcie->cra_base);
> + if ((pcie->pcie_data->version == ALTERA_PCIE_V1) ||
> + (pcie->pcie_data->version == ALTERA_PCIE_V2)) {
> + pcie->cra_base =
> + devm_platform_ioremap_resource_byname(pdev, "Cra");
> + if (IS_ERR(pcie->cra_base))
> + return PTR_ERR(pcie->cra_base);
> + }
Should be a separate introductory patch since it's not directly
related to Agilex.
> - if (pcie->pcie_data->version == ALTERA_PCIE_V2) {
> + if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> + pcie->cs_base =
> + devm_platform_ioremap_resource_byname(pdev, "Cs");
> + if (IS_ERR(pcie->cs_base))
> + return PTR_ERR(pcie->cs_base);
> + of_property_read_u32(pcie->pdev->dev.of_node, "port_conf_stat",
> + &port_conf_off);
> + dev_info(&pcie->pdev->dev, "port_conf_stat_off =%x\n",
> + port_conf_off);
> + }
> +
> + if ((pcie->pcie_data->version == ALTERA_PCIE_V2) ||
> + (pcie->pcie_data->version == ALTERA_PCIE_V3)) {
Ditto.
> pcie->hip_base =
> devm_platform_ioremap_resource_byname(pdev, "Hip");
> if (IS_ERR(pcie->hip_base))
> @@ -708,7 +936,8 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
> if (pcie->irq < 0)
> return pcie->irq;
>
> - irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
> + irq_set_chained_handler_and_data(pcie->irq,
> + pcie->pcie_data->ops->rp_isr, pcie);
Ditto (including the new .rp_isr initializations below).
> @@ -721,6 +950,7 @@ static const struct altera_pcie_ops altera_pcie_ops_1_0 = {
> .tlp_read_pkt = tlp_read_packet,
> .tlp_write_pkt = tlp_write_packet,
> .get_link_status = altera_pcie_link_up,
> + .rp_isr = altera_pcie_isr,
> };
> @@ -793,11 +1045,17 @@ static int altera_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> - /* 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);
Ditto.
> + } else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> + writel(CFG_AER, (pcie->hip_base + port_conf_off
> + + AGLX_ROOT_PORT_IRQ_ENABLE));
> + }
>
> bridge->sysdata = pcie;
> bridge->busnr = pcie->root_bus_nr;
> --
> 2.34.1
>
