Message-ID: <000d01dc1022$ad8c0740$08a415c0$@samsung.com>
Date: Mon, 18 Aug 2025 15:00:00 +0530
From: "Shradha Todi" <shradha.t@...sung.com>
To: "'Bjorn Helgaas'" <helgaas@...nel.org>
Cc: <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-samsung-soc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
	<mani@...nel.org>, <lpieralisi@...nel.org>, <kwilczynski@...nel.org>,
	<robh@...nel.org>, <bhelgaas@...gle.com>, <jingoohan1@...il.com>,
	<krzk+dt@...nel.org>, <conor+dt@...nel.org>, <alim.akhtar@...sung.com>,
	<vkoul@...nel.org>, <kishon@...nel.org>, <arnd@...db.de>,
	<m.szyprowski@...sung.com>, <jh80.chung@...sung.com>,
	<pankaj.dubey@...sung.com>
Subject: RE: [PATCH v3 11/12] PCI: exynos: Add support for Tesla FSD SoC

> 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@samsung.com/)
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

Powered by blists - more mailing lists