Re: [RFC PATCH 2/2] PCI: Add Qualcomm PCIe ECAM root complex driver

[Bjorn Helgaas <helgaas@kernel.org>]

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.

> 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.

> +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.

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Does this actually work?  I expected "#define dev_fmt" since you're
using dev_err(), etc below.

> +#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/

> +#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?

> + * 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/.

> +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.

> +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.

> +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.

> +static struct msi_domain_ops qcom_msi_domain_ops = {
> +	.msi_prepare	= qcom_msi_domain_prepare,

Rename so function name includes the struct member name.

> +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.

> +	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;

> +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.

> +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.

> +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.