> On Mon, Aug 11, 2025 at 09:16:37PM +0530, Shradha Todi wrote: > > 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@xxxxxxxxxxx/) 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" > > +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. > > + 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. > > +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 > > + 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
