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

[Mayank Rana <quic_mrana@quicinc.com>]

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