On Mon, Aug 11, 2025 at 09:16:37PM +0530, Shradha Todi wrote:

quoted

Add host and endpoint controller driver support for FSD SoC.

I think this might be easier if you added host mode first, then added
endpoint mode with a separate patch.

Will do.

It's kind of unfortunate that the driver uses "ep" everywhere for
struct exynos_pcie pointers.  It's going to be confusing because "ep"
is also commonly used for endpoint-related things, e.g., struct
dw_pcie_ep pointers.  Maybe it's not worth changing; I dunno.

I did try to rename the structure and the pointers 
(https://lore.kernel.org/all/20230214121333.1837-9-shradha.t@samsung.com/ (local))
But the intention was different back then and so the idea was rejected.
I could add a patch to only rename the pointers to something less
confusing like "exy_pci"

quoted

+static irqreturn_t fsd_pcie_irq_handler(int irq, void *arg)
+{
+	u32 val;
+	struct exynos_pcie *ep = arg;
+	struct dw_pcie *pci = &ep->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+
+	val = readl(ep->elbi_base + FSD_IRQ2_STS);
+	if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
+		val &= FSD_IRQ_MSI_ENABLE;
+		writel(val, ep->elbi_base + FSD_IRQ2_STS);

This looks weird because FSD_IRQ_MSI_ENABLE sounds like an *enable*
bit, but here you're treating it as a *status* bit.

As far as I can tell, you set FSD_IRQ_MSI_ENABLE once at probe-time in
fsd_pcie_msi_init(), then you clear it here in an IRQ handler, and it
will never be set again.  That seems wrong; am I missing something?

Actually the status IRQ and enable IRQ registers are different offsets
but the bit position for MSI remains same in both cases so I just reused
the macro. But I understand that it's confusing so I will add another
macro for FSD_IRQ_MSI_STATUS or just rename the macro to
FSD_IRQ_MSI to re-use.

quoted

+		dw_handle_msi_irq(pp);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void fsd_pcie_msi_init(struct exynos_pcie *ep)
+{
+	int val;
+
+	val = readl(ep->elbi_base + FSD_IRQ2_EN);
+	val |= FSD_IRQ_MSI_ENABLE;
+	writel(val, ep->elbi_base + FSD_IRQ2_EN);
+}
+
+static void __iomem *fsd_atu_setting(struct dw_pcie *pci, void __iomem *base)

The "setting" name suggests that this merely returns an address
without side effects, but in fact it actively *sets* the view.

In this case there's no locking around:

  addr = fsd_atu_setting(pci, base);
  dw_pcie_read(addr + reg, size, &val);

even though concurrent calls would cause issues, but I think that's OK
because we only get there via the driver, and I assume multiple DBI or
DBI2 accesses never happen because they're not used in asynchronous
paths like interrupt handlers.

Yes, there is no concurrent access to this function and hence I have
not added locking mechanism.

But I think a name that hints at the fact that this does have side
effects would be helpful as a reminder in the callers that they must
not be used concurrently.

Sure, I will change the name and also add comment as a reminder.

quoted

+static const struct pci_epc_features fsd_pcie_epc_features = {
+	.linkup_notifier = false,
+	.msi_capable = true,
+	.msix_capable = false,

I think we should omit features we do *not* support instead of calling
them out explicitly, e.g., we don't need .linkup_notifier or
.msix_capable.

We've added them in the past, but they're unnecessary and they lead to
either pervasive changes (adding ".new_feature = false" to all
existing drivers when adding the feature) or inconsistency (new
drivers include ".new_feature = false" but existing drivers do not).

Will remove

quoted

+	if (ep->pdata->soc_variant == FSD) {
+		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
+		if (ret)
+			return ret;
+
+		ep->sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
+				"samsung,syscon-pcie");
+		if (IS_ERR(ep->sysreg)) {
+			dev_err(dev, "sysreg regmap lookup failed.\n");
+			return PTR_ERR(ep->sysreg);
+		}
+
+		ret = of_property_read_u32_index(dev->of_node, "samsung,syscon-pcie", 1,
+						 &ep->sysreg_offset);
+		if (ret) {
+			dev_err(dev, "couldn't get the register offset for syscon!\n");
+			return ret;
+		}
+	}

This is a good example of a complicated set of things where I think
you should either add a SoC-specific function pointer to do this or
test a property, e.g., "DMA width", instead of testing for a specific
SoC.

Got your point and it makes sense. In future, other drivers could also
want to set DMA width, etc. Will make properties to replace soc_variant:
 - DMA_width
 - has_syscon
 - function pointer to assert_core_reset and deassert_core_reset

Any suggestions or is this approach okay?

-Shradha