Received-SPF: pass (google.com: domain of linux-kernel+bounces-179178-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 14 May 2024 16:14:52 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
Cc: lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
	bhelgaas@google.com, manivannan.sadhasivam@linaro.org,
	fancer.lancer@gmail.com, u.kleine-koenig@pengutronix.de,
	cassel@kernel.org, dlemoal@kernel.org,
	yoshihiro.shimoda.uh@renesas.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	srk@ti.com
Subject: Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC
Message-ID: <20240514211452.GA2082543@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8b56604d-a2b8-4227-8a6f-c477332416b4@ti.com>

On Tue, May 14, 2024 at 05:41:48PM +0530, Siddharth Vadapalli wrote:
> On Mon, May 13, 2024 at 04:53:50PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 28, 2024 at 02:20:41PM +0530, Siddharth Vadapalli wrote:
> > > In the process of converting .scan_bus() callbacks to .add_bus(), the
> > > ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus().
> > > The .scan_bus() method belonged to ks_pcie_host_ops which was specific
> > > to controller version 3.65a, while the .add_bus() method had been added
> > > to ks_pcie_ops which is shared between the controller versions 3.65a and
> > > 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer
> > > ks_pcie_v3_65_add_bus() method is applicable to the controller version
> > > 4.90a which is present in AM654x SoCs.
> > > 
> > > Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents
> > > to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to
> > > the 3.65a controller.
> > > 
> > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus")
> > > Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> > > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > > Suggested-by: Niklas Cassel <cassel@kernel.org>
> > > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > 
> > Thanks for splitting this into two patches.  Krzysztof has applied
> > both to pci/controller/keystone and we hope to merge them for v6.10.
> > 
> > I *would* like the commit log to be at a little higher level if
> > possible.  Right now it's a detailed description at the level of the
> > code edits, but it doesn't say *why* we want this change.
> > 
> > I think the first cut at this was
> > https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u,
> > which mentioned Completion Timeouts during MSI-X configuration and 45
> > second delays during boot.
> > 
> > IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR
> > 0 and was only used for v3.65a devices.  6ab15b5e7057 renamed it to
> > ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a.
> > 
> > I think the problem is that in the current code, the
> > ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all
> > devices (both v3.65a and v4.90a).  So I guess doing the BAR 0 setup on
> > v4.90a broke something there?
> 
> BAR0 was set to a different value on AM654x SoC which has the v4.90a
> controller, which is identical to what is set even for the v3.65a
> controller. The difference is that BAR0 is programmed to a different
> value for enabling inbound MSI writes on top of the common configuration
> performed for BAR0.
> 
> Common configuration for BAR0:
> ks_pcie_probe
>   dw_pcie_host_init
>     dw_pcie_setup_rc
>     ...
>      /* Setup RC BARs */
>      dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
>      dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000);

Tangential questions that don't need to be addressed for this patch:
There's a bunch of code between these writes and the one below.  Does
that code in the middle depend on the above, or should all three of
these writes be together?  Is there some magic in writing
PCI_BASE_ADDRESS_0 twice?  This is common code for all DWC devices, so
it must work OK, but it looks strange.

>      dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);

What's the net effect of these three writes?  Disabling BAR 0 and
BAR 1, as suggested at [1, 2]?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-keystone.c?id=v6.9#n399
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-rcar-gen4.c?id=v6.9#n280

> MSI specific configuration of BAR0 performed after the common
> configuration via the ks_pcie_v3_65_scan_bus() callback:
> 	/* Configure and set up BAR0 */
> 	ks_pcie_set_dbi_mode(ks_pcie);
> 
> 	/* Enable BAR0 */
> 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1);
> 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1);

I assume that in DBI mode, this actually writes a mask, not a value?
If writing a 0xfff mask means that the low 12 bits could not be set,
maybe this configures it to be a 4K memory BAR?

But is there some reason to do two writes to the same register?

> 	ks_pcie_clear_dbi_mode(ks_pcie);
> 
> 	 /*
> 	  * For BAR0, just setting bus address for inbound writes (MSI) should
> 	  * be sufficient.  Use physical address to avoid any conflicts.
> 	  */
> 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start);

After clearing DBI mode, this looks like it would set BAR 0 to the
ks_pcie->app.start address, which makes some sense, since the low 4
bits would all be zero (assuming the address is page aligned), which
would make it a 32-bit memory BAR.

> The above configuration of BAR0 shouldn't be performed for AM654x SoC.
> While I am not certain, the timeouts are probably a result of the BAR
> being programmed to a wrong value which results in a "no match" outcome.
> 
> > I'm not quite clear on the mechanism, but it would be helpful to at
> > least know what's wrong and on what platform.  E.g., currently v4.90
> > suffers Completion Timeouts and 45 second boot delays?  And this patch
> > fixes that?
> 
> Yes, the Completion Timeouts cause the 45 second boot delays and this
> patch fixes that.

And this problem happens on AM654x/v4.90a, right?  I really want the
commit log to say what platform is affected!

Maybe something like this?

  PCI: keystone: Enable BAR 0 only for v3.65a

  The BAR 0 initialization done by ks_pcie_v3_65_add_bus() should
  happen for v3.65a devices only.  On other devices, BAR 0 should be
  left disabled, as it is by dw_pcie_setup_rc().

  After 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus()
  callback to use add_bus"), ks_pcie_v3_65_add_bus() enabled BAR 0 for
  both v3.65a and v4.90a devices.  On the AM654x SoC, which uses
  v4.90a, enabling BAR 0 causes Completion Timeouts when setting up
  MSI-X.  These timeouts delay boot of the AM654x by about 45 seconds.

  Move the BAR 0 initialization to ks_pcie_msi_host_init(), which is
  only used for v3.65a devices, and remove ks_pcie_v3_65_add_bus().