Hi Bjorn
Thanks for reviewing change.
On 4/5/2024 11:30 AM, Bjorn Helgaas wrote:
On Thu, Apr 04, 2024 at 12:11:24PM -0700, Mayank Rana wrote:
On some of Qualcomm platform, firmware configures PCIe controller into
ECAM mode allowing static memory allocation for configuration space of
supported bus range. Firmware also takes care of bringing up PCIe PHY
and performing required operation to bring PCIe link into D0. Firmware
I think link state would be L0, not D0.
ACK
also manages system resources (e.g. clocks/regulators/resets/ bus voting).
Hence add Qualcomm PCIe ECAM root complex driver which enumerates PCIe
root complex and connected PCIe devices. Firmware won't be enumerating
or powering up PCIe root complex until this driver invokes power domain
based notification to bring PCIe link into D0/D3cold mode.
Again.
ACK. will repharse it.
+config PCIE_QCOM_ECAM
+ tristate "QCOM PCIe ECAM host controller"
+ depends on ARCH_QCOM && PCI
+ depends on OF
+ select PCI_MSI
+ select PCI_HOST_COMMON
+ select IRQ_DOMAIN
+ help
+ Say 'Y' here if you want to use ECAM shift mode compatible Qualcomm
+ PCIe root host controller. The controller is programmed using firmware
+ to support ECAM compatible memory address space.
Instead of adding this at the end, place this entry so the entire list
remains sorted by vendor name.
Other related entries are "Qualcomm PCIe controller ..." (not "QCOM").
Use "ECAM PCIe controller (host mode)" (not "PCIe ECAM host
controller") so it matches similar entries.
Ok. will rephrase and move as suggested.
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Does this actually work? I expected "#define dev_fmt" since you're
using dev_err(), etc below.
Yes. look like it is not needed anymore for this driver as very limited
pr_* usage.
+#include <linux/irqchip/chained_irq.h>
Can this be reworked so it doesn't use chained IRQs? I admit to not
being an IRQ expert, but I have the impression that it's better to
avoid the chained IRQ model when possible. See
https://lore.kernel.org/all/20231108153133.GA393726@bhelgaas/
Ok. will review shared information, and try to rework upon this.
+#define MSI_DB_ADDR 0xa0000000
Where does this come from and why is it hard-coded here? Looks like a
magic address that maybe should come from DT?
Yes it is DB address to generate MSI, and it is not tied with directly
with any
hardware/platform. Hence hardcoding here.
+ * struct qcom_msi_irq - MSI IRQ information
+ * @client: pointer to MSI client struct
+ * @grp: group the irq belongs to
s/irq/IRQ/ in comments for consistency (other occurrences below).
Same for s/pcie/PCIe/ and s/msi/MSI/.
ACK
+static void qcom_msi_mask_irq(struct irq_data *data)
+{
+ struct irq_data *parent_data;
+ struct qcom_msi_irq *msi_irq;
+ struct qcom_msi_grp *msi_grp;
+ struct qcom_msi *msi;
+ unsigned long flags;
+
+ parent_data = data->parent_data;
+ if (!parent_data)
+ return;
Drop this test; I think it only detects logic errors in the driver or
memory corruptions, and we want to find out about those.
ACK
+static void qcom_msi_unmask_irq(struct irq_data *data)
+{
+ struct irq_data *parent_data;
+ struct qcom_msi_irq *msi_irq;
+ struct qcom_msi_grp *msi_grp;
+ struct qcom_msi *msi;
+ unsigned long flags;
+
+ parent_data = data->parent_data;
+ if (!parent_data)
+ return;
Drop.
ACK
+static struct irq_chip qcom_msi_irq_chip = {
+ .name = "qcom_pci_msi",
+ .irq_enable = qcom_msi_unmask_irq,
+ .irq_disable = qcom_msi_mask_irq,
+ .irq_mask = qcom_msi_mask_irq,
+ .irq_unmask = qcom_msi_unmask_irq,
Name these so they match the struct member, e.g., the name should
contain "irq_mask", not "mask_irq") so grep finds them easily.
ACK
+static struct msi_domain_ops qcom_msi_domain_ops = {
+ .msi_prepare = qcom_msi_domain_prepare,
Rename so function name includes the struct member name.
ACK
+static int qcom_msi_irq_set_affinity(struct irq_data *data,
+ const struct cpumask *mask, bool force)
+{
+ struct irq_data *parent_data = irq_get_irq_data(irqd_to_hwirq(data));
+ int ret = 0;
+
+ if (!parent_data)
+ return -ENODEV;
+
+ /* set affinity for MSI HW IRQ */
Unnecessary comment.
ACK
+ if (parent_data->chip->irq_set_affinity)
+ ret = parent_data->chip->irq_set_affinity(parent_data, mask, force);
+
+ return ret;
Drop "ret" and return directly, e.g.,
if (parent_data->chip->irq_set_affinity)
return parent_data->chip->irq_set_affinity(...);
return 0;
ACK
+static void qcom_msi_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+ struct irq_data *parent_data = irq_get_irq_data(irqd_to_hwirq(data));
+ struct qcom_msi_irq *msi_irq = irq_data_get_irq_chip_data(data);
+ struct qcom_msi_client *client = msi_irq->client;
+
+ if (!parent_data)
+ return;
Drop.
ACK
+static void qcom_msi_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs)
+{
+ struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+ struct qcom_msi_client *client;
+ struct qcom_msi_irq *msi_irq;
+ struct qcom_msi *msi;
+
+ if (!data)
+ return;
Drop.
ACK
+static int qcom_msi_irq_setup(struct qcom_msi *msi)
+{
+ struct qcom_msi_grp *msi_grp;
+ struct qcom_msi_irq *msi_irq;
+ int i, index, ret;
+ unsigned int irq;
+
+ /* setup each MSI group. nr_hwirqs == nr_grps */
+ for (i = 0; i < msi->nr_hwirqs; i++) {
+ irq = irq_of_parse_and_map(msi->dev->of_node, i);
+ if (!irq) {
+ dev_err(msi->dev,
+ "MSI: failed to parse/map interrupt\n");
Possibly include "i" to identify the offending entry.
ACK
Regards,
Mayank
