Re: [PATCH] PCI: dwc: Separate CFG0 and CFG1 into different ATU regions

From: Shawn Guo <shawn.guo@linaro.org>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Pratyush Anand <pratyush.anand@gmail.com>,
	Zaihai Yu <yuzaihai@hisilicon.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] PCI: dwc: Separate CFG0 and CFG1 into different ATU regions
Date: Thu, 9 Jan 2020 19:14:58 +0800	[thread overview]
Message-ID: <20200109111457.GA18850@T480> (raw)
In-Reply-To: <CH2PR12MB40073FCB953227A37F7A1A91DA390@CH2PR12MB4007.namprd12.prod.outlook.com>

Hi Gustavo,

Thanks for taking a look.

On Thu, Jan 09, 2020 at 10:37:14AM +0000, Gustavo Pimentel wrote:
> Hi Shawn,
> 
> On Thu, Jan 9, 2020 at 6:6:57, Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> > Some platform has 4 (or more) viewports.  In that case, CFG0 and CFG1
> 
> Remove double space before "In that..." 

Hmm, that was intentional.  My writing practice is using two spaces
after a period and single space after a comma.  Is it a bad habit?

@Lorenzo, @Bjorn, let me know if you guys think this should be fixed.

> 
> > can be separated into different ATU regions.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c    | 16 +++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 0f36a926059a..504d2a192bba 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -532,6 +532,7 @@ static int dw_pcie_access_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> >  	u64 cpu_addr;
> >  	void __iomem *va_cfg_base;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	int index = PCIE_ATU_REGION_INDEX1;
> >  
> >  	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> >  		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
> > @@ -548,7 +549,20 @@ static int dw_pcie_access_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> >  		va_cfg_base = pp->va_cfg1_base;
> >  	}
> >  
> > -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> > +	if (pci->num_viewport >= 4) {
> > +		/*
> > +		 * If there are 4 (or more) viewports, we can separate
> > +		 * CFG0 and CFG1 into different ATU regions:
> > +		 *  - region0: MEM
> > +		 *  - region1: CFG0
> > +		 *  - region2: IO
> > +		 *  - region3: CFG1
> > +		 */
> > +		if (type == PCIE_ATU_TYPE_CFG1)
> > +			index = PCIE_ATU_REGION_INDEX3;
> > +	}
> > +
> > +	dw_pcie_prog_outbound_atu(pci, index,
> >  				  type, cpu_addr,
> >  				  busdev, cfg_size);
> >  	if (write)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 5a18e94e52c8..86225804f1e7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -63,6 +63,7 @@
> >  #define PCIE_ATU_VIEWPORT		0x900
> >  #define PCIE_ATU_REGION_INBOUND		BIT(31)
> >  #define PCIE_ATU_REGION_OUTBOUND	0
> > +#define PCIE_ATU_REGION_INDEX3		0x3
> >  #define PCIE_ATU_REGION_INDEX2		0x2
> >  #define PCIE_ATU_REGION_INDEX1		0x1
> >  #define PCIE_ATU_REGION_INDEX0		0x0
> > -- 
> > 2.17.1
> 
> With the description fix,
> 
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Thanks!

Shawn