On Mon, Sep 20, 2021 at 12:29:45PM +0530, Manivannan Sadhasivam wrote:
> Add driver support for Qualcomm PCIe Endpoint controller driver based on
> the Designware core with added Qualcomm specific wrapper around the
> core.
> ...
> +static irqreturn_t qcom_pcie_ep_perst_irq_thread(int irq, void *data)
> +{
> + struct qcom_pcie_ep *pcie_ep = data;
> + struct dw_pcie *pci = &pcie_ep->pci;
> + struct device *dev = pci->dev;
> + u32 perst;
> +
> + perst = gpiod_get_value(pcie_ep->reset);
> + if (perst) {
> + dev_dbg(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> + qcom_pcie_perst_assert(pci);
> + } else {
> + dev_dbg(dev, "PERST de-asserted by host. Starting link training!\n");
> + qcom_pcie_perst_deassert(pci);
> + }
> +
> + irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> + (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW));
1) There are only a handful of instances of irq_set_irq_type() being
used with IRQF_TRIGGER_* (all others use IRQ_TYPE_*).
2) Using irq_set_irq_type() in an IRQ handler is unusual and seems
potentially racy. Almost all irq_set_irq_type() uses are in
initialization or probe paths. I did see one similar use in an IRQ
handler (rb532_pata_irq_handler()), but the rarity of this pattern
makes me suspicious.
> +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> + struct qcom_pcie_ep *pcie_ep)
> +{
> + ...
> + pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> + irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> + ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> + qcom_pcie_ep_perst_irq_thread,
> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> + "perst_irq", pcie_ep);
The similar code in the tegra194 driver looks like this:
tegra_pcie_config_ep
devm_request_threaded_irq(tegra_pcie_ep_pex_rst_irq,
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
tegra_pcie_ep_pex_rst_irq
if (gpiod_get_value(pcie->pex_rst_gpiod))
pex_ep_event_pex_rst_assert(pcie);
else
pex_ep_event_pex_rst_deassert(pcie);
Could qcom work the same way by requesting the IRQ with
"IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING" instead of
"IRQF_TRIGGER_HIGH", and omitting the irq_set_irq_type()?
I know rising/falling is edge-triggered and high/low is
level-triggered, but surely qcom isn't completely unique in the way
its IRQ is wired up?
