From: Bjorn Helgaas <helgaas@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: kishon@ti.com, lorenzo.pieralisi@arm.com, bhelgaas@google.com,
robh@kernel.org, devicetree@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, hemantk@codeaurora.org,
bjorn.andersson@linaro.org, sallenki@codeaurora.org,
skananth@codeaurora.org, vpernami@codeaurora.org,
vbadigan@codeaurora.org,
Siddartha Mohanadoss <smohanad@codeaurora.org>
Subject: Re: [PATCH v8 2/3] PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver
Date: Thu, 15 Aug 2024 17:47:35 -0500 [thread overview]
Message-ID: <20240815224735.GA57931@bhelgaas> (raw)
In-Reply-To: <20210920065946.15090-3-manivannan.sadhasivam@linaro.org>
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?
next prev parent reply other threads:[~2024-08-15 22:47 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-20 6:59 [PATCH v8 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
2021-09-20 6:59 ` [PATCH v8 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
2021-09-20 6:59 ` [PATCH v8 2/3] PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
2024-08-15 22:47 ` Bjorn Helgaas [this message]
2021-09-20 6:59 ` [PATCH v8 3/3] MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding Manivannan Sadhasivam
2021-10-04 4:19 ` [PATCH v8 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
2021-10-07 12:57 ` Manivannan Sadhasivam
2021-10-07 13:12 ` Lorenzo Pieralisi
2021-10-07 13:53 ` Lorenzo Pieralisi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240815224735.GA57931@bhelgaas \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=hemantk@codeaurora.org \
--cc=kishon@ti.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=manivannan.sadhasivam@linaro.org \
--cc=robh@kernel.org \
--cc=sallenki@codeaurora.org \
--cc=skananth@codeaurora.org \
--cc=smohanad@codeaurora.org \
--cc=vbadigan@codeaurora.org \
--cc=vpernami@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.