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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).