On Wed, Aug 19, 2020 at 04:33:10PM +0000, Daire.McNamara@xxxxxxxxxxxxx wrote:
> + * pcie-altera.c
> + */
Add a blank line here.
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/module.h>
> +static struct mc_port *port;
This file scope item is not ideal. It might work in your situation if
you can never have more than one device, but it's not a pattern we
want other people to copy.
I think I sort of see how it works:
mc_pci_host_probe
pci_host_common_probe
ops = of_device_get_match_data() # mc_ecam_ops
gen_pci_init(..., ops)
pci_ecam_create(..., ops)
ops->init # mc_ecam_ops.init
mc_platform_init(pci_config_window *)
port = devm_kzalloc(...) # initialized
mc_setup_windows
bridge_base_addr = port->axi_base_addr + ... # used
And you're using the file scope "port" because mc_platform_init()
doesn't have a pointer to the platform_device. But I think this
abuses the pci_ecam_ops design to do host bridge initialization that
it is not intended for.
I'd suggest copying the host bridge init design from somewhere else.
tango_pcie_probe() also calls pci_host_common_probe(), so maybe that's
a good place. But the fact that it's the *only* such caller makes me
think it might not be the best thing to copy.
Rob has been working in this area and probably has better insight.
> +static void mc_pcie_isr(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct mc_port *port = irq_desc_get_handler_data(desc);
> + struct device *dev = port->dev;
> + struct mc_msi *msi = &port->msi;
> + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> + unsigned long status;
> + unsigned long intx_status;
> + unsigned long msi_status;
> + u32 bit;
> + u32 virq;
> +
> + /*
> + * The core provides a single interrupt for both INTx/MSI messages.
> + * So we'll read both INTx and MSI status.
> + */
> + chained_irq_enter(chip, desc);
> +
> + status = readl_relaxed(bridge_base_addr + MC_ISTATUS_LOCAL);
readl_relaxed() is defined to return a u32, so you might as well
declare "status" and related things as u32 instead of "unsigned long".
> +static void mc_pcie_enable_msi(struct mc_port *port, void __iomem *base)
> +{
> + struct mc_msi *msi = &port->msi;
> + u32 cap_offset = MC_MSI_CAP_CTRL_OFFSET;
> +
Remove this blank line. We usually just separate the automatic
variables from the code with a blank line.
> + u16 msg_ctrl = readw_relaxed(base + cap_offset + PCI_MSI_FLAGS);
> +static void mc_mask_intx_irq(struct irq_data *data)
> +{
> + struct mc_port *port = irq_data_get_irq_chip_data(data);
> + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = PCIE_LOCAL_INT_ENABLE;
> + raw_spin_lock_irqsave(&port->intx_mask_lock, flags);
> + val = readl_relaxed(bridge_base_addr + MC_IMASK_LOCAL);
> + val &= ~mask;
Why bother with "mask" here? It's only used once; just use
PCIE_LOCAL_INT_ENABLE directly.
> + writel_relaxed(val, bridge_base_addr + MC_IMASK_LOCAL);
> + raw_spin_unlock_irqrestore(&port->intx_mask_lock, flags);
> +}
> +
> +static void mc_unmask_intx_irq(struct irq_data *data)
> +{
> + struct mc_port *port = irq_data_get_irq_chip_data(data);
> + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> + unsigned long flags;
> + u32 mask, val;
> +
> + mask = PCIE_LOCAL_INT_ENABLE;
> + raw_spin_lock_irqsave(&port->intx_mask_lock, flags);
> + val = readl_relaxed(bridge_base_addr + MC_IMASK_LOCAL);
> + val |= mask;
Here also.
> +static int mc_platform_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct platform_device *pdev = to_platform_device(dev);
> + void __iomem *bridge_base_addr;
> + void __iomem *ctrl_base_addr;
> + int ret;
> + u32 irq;
> + u32 val;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> + port->dev = dev;
> +
> + port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(port->axi_base_addr))
> + return PTR_ERR(port->axi_base_addr);
> +
> + bridge_base_addr = port->axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> + ctrl_base_addr = port->axi_base_addr + MC_PCIE1_CTRL_ADDR;
> +
> + port->msi.vector_phy = MC_MSI_ADDR;
> + port->msi.num_vectors = MC_NUM_MSI_IRQS;
> + ret = mc_pcie_init_irq_domains(port);
> + if (ret) {
> + dev_err(dev, "failed creating IRQ domains\n");
> + return ret;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
platform_get_irq() returns "int". You declared "irq" as "u32" above,
so this check for failure won't work correctly.
> + dev_err(dev, "unable to request IRQ%d\n", irq);
> + return -ENODEV;
> + }
