Message-ID: <20220628114226.frdsgk2xe2o7z5xx@mobilestation>
Date:   Tue, 28 Jun 2022 14:42:26 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Frank Li <Frank.Li@....com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jonathan Chocron <jonnyc@...zon.com>,
        Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH RESEND v5 03/18] PCI: dwc: Disable outbound windows for
 controllers with iATU

Hi Bjorn

On Mon, Jun 27, 2022 at 05:40:58PM -0500, Bjorn Helgaas wrote:
> [+cc Jonathan for pcie-al.c, Kishon for pci-keystone.c]
> 
> On Fri, Jun 24, 2022 at 05:34:13PM +0300, Serge Semin wrote:
> > In accordance with the dw_pcie_setup_rc() method semantics and judging by
> > what the comment added in commit dd193929d91e ("PCI: designware: Explain
> > why we don't program ATU for some platforms") states there are DWC
> > PCIe-available platforms like Keystone (pci-keystone.c) or Amazon's
> > Annapurna Labs (pcie-al.c) which don't have the DW PCIe internal ATU
> > enabled and use it's own address translation approach implemented. In
> > these cases at the very least there is no point in touching the DW PCIe
> > iATU CSRs. Moreover depending on the vendor-specific address translation
> > implementation it might be even erroneous. So let's move the iATU windows
> > disabling procedure to being under the corresponding conditional statement
> > clause thus performing that procedure only if the iATU is expected to be
> > available on the platform.
> 

> Added Jonathan and Kishon to make sure pcie-al.c and pci-keystone.c
> (the only two drivers that override the default dw_child_pcie_ops)
> won't be broken by skipping the outbound window disable.

Makes sense. Thanks.

> 
> > Fixes: 458ad06c4cdd ("PCI: dwc: Ensure all outbound ATU windows are reset")
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > Reviewed-by: Rob Herring <robh@...nel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index bc9a7df130ef..d4326aae5a32 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -543,7 +543,6 @@ static struct pci_ops dw_pcie_ops = {
> >  
> >  void dw_pcie_setup_rc(struct pcie_port *pp)
> >  {
> > -	int i;
> >  	u32 val, ctrl, num_ctrls;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >  
> > @@ -594,19 +593,22 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
> >  	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
> >  
> > -	/* Ensure all outbound windows are disabled so there are multiple matches */
> > -	for (i = 0; i < pci->num_ob_windows; i++)
> > -		dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
> > -
> >  	/*
> >  	 * If the platform provides its own child bus config accesses, it means
> >  	 * the platform uses its own address translation component rather than
> >  	 * ATU, so we should not program the ATU here.
> >  	 */
> >  	if (pp->bridge->child_ops == &dw_child_pcie_ops) {
> > -		int atu_idx = 0;
> > +		int i, atu_idx = 0;
> >  		struct resource_entry *entry;
> >  
> > +		/*
> > +		 * Ensure all outbound windows are disabled so there are
> > +		 * multiple matches
> 

> I know you only moved this comment and didn't change the wording, but
> do you know what it means?  What "multiple matches" is it talking
> about, and why are they important?

AFAIU the In/Out-bound windows disabling procedure is a kind of
cleanup-before-usage pattern. So if the DW PCIe controller has been
used by a bootloader with non-DT-based (dma-)ranges setup, and hasn't
been reset by the low-level driver, some windows can be left
opened/configured. It may cause unpleasant side effects on the further
controller utilization. Although I don't see much room for the bugs in
this part since the iATU setup is overwritten from lowest index to the
highest one afterwards anyway and in accordance with the HW ref.
manual the first-match region will be used for the CPU<->PCIe IOs
translation. Basically should we leave the in/out-bound windows setup
uncleared the only thing that may cause problems/unexpected IO results
is the not-overridden iATU regions. It won't be that much of the
problem, but more like an unexpected behaviour, for instance, a random
MWr/MRd TLPs (depending on the bootloader iATU setup) generation on an
attempt to access some MMIO ranges, which aren't supposed to be used
by the system anyway.

Anyway as Rob said in the commit 458ad06c4cdd ("PCI: dwc: Ensure all
outbound ATU windows are reset") indeed it won't hurt to perform the
cleanup. It shall make the system state more predictable.

> 
> I guess Rob previously moved it with 458ad06c4cdd ("PCI: dwc: Ensure
> all outbound ATU windows are reset") [1], and it looks like maybe the
> point is to *avoid* having an outbound transaction match multiple
> windows?  So maybe this comment should say this?
> 
>   Disable all outbound windows to make sure a transaction can't match
>   multiple windows.

You are right. I can fix the text on v6 of the series.

-Sergey

> 
> [1] https://git.kernel.org/linus/458ad06c4cdd
> 
> > +		 */
> > +		for (i = 0; i < pci->num_ob_windows; i++)
> > +			dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
> > +
> >  		/* Get last memory resource entry */
> >  		resource_list_for_each_entry(entry, &pp->bridge->windows) {
> >  			if (resource_type(entry->res) != IORESOURCE_MEM)
> > -- 
> > 2.35.1
> > 

Powered by blists - more mailing lists