PCI: dwc: Separate CFG0 and CFG1 into different ATU regions
diff mbox series

Message ID 20200109060657.1953-1-shawn.guo@linaro.org
State New
Headers show
Series
  • PCI: dwc: Separate CFG0 and CFG1 into different ATU regions
Related show

Commit Message

Shawn Guo Jan. 9, 2020, 6:06 a.m. UTC
Some platform has 4 (or more) viewports.  In that case, CFG0 and CFG1
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(-)

Comments

Gustavo Pimentel Jan. 9, 2020, 10:37 a.m. UTC | #1
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..." 

> 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>
Shawn Guo Jan. 9, 2020, 11:14 a.m. UTC | #2
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
Russell King - ARM Linux admin Jan. 9, 2020, 11:28 a.m. UTC | #3
On Thu, Jan 09, 2020 at 07:14:58PM +0800, Shawn Guo wrote:
> 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?

Mine too.  It is not a bad habit, it's just a different style, but
some people can't cope with other people having different styles
and feel an urge to try and make them conform to their own ideals.
At the end of the day, it is personal style, and should *not* be
commented on, IMHO.
Gustavo Pimentel Jan. 9, 2020, 12:24 p.m. UTC | #4
On Thu, Jan 9, 2020 at 11:14:58, Shawn Guo <shawn.guo@linaro.org> wrote:

> 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?

I thought it was a typo. I personally don't have anything against it, but 
I didn't see this style on the comments till now. To keep the coherence 
between all patches, I know that Bjorn and Lorenzo like to have it the 
most standardized possible. It is OK by Lorenzo and Bjorn, it's fine for 
me too.

> 
> @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
Vidya Sagar Jan. 9, 2020, 5:34 p.m. UTC | #5
On 1/9/2020 11:36 AM, Shawn Guo wrote:
> External email: Use caution opening links or attachments
> 
> 
> Some platform has 4 (or more) viewports.  In that case, CFG0 and CFG1
> can be separated into different ATU regions.
Is there any specific benefit with this scheme?

- Vidya Sagar
> 
> 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
>
Bjorn Helgaas Jan. 9, 2020, 6:29 p.m. UTC | #6
On Thu, Jan 09, 2020 at 12:24:17PM +0000, Gustavo Pimentel wrote:
> On Thu, Jan 9, 2020 at 11:14:58, Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> > 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?
> 
> I thought it was a typo. I personally don't have anything against it, but 
> I didn't see this style on the comments till now. To keep the coherence 
> between all patches, I know that Bjorn and Lorenzo like to have it the 
> most standardized possible. It is OK by Lorenzo and Bjorn, it's fine for 
> me too.

Eagle eyes!  I was taught in the dark ages of typewriters to use two
spaces after a period, but I don't really care either way.  If I
rework a commit log for other reasons I might use two spaces, and I
frequently use vim 'gq' to reformat paragraphs to use the whole line
width, and I think that inserts two spaces (by default), so I try to
be consistent at least within each commit log.  But either is really
fine with me.

Thanks for taking the time to read and pay attention to commit logs!

Bjorn
Shawn Guo Jan. 10, 2020, 8:56 a.m. UTC | #7
Hi Vidya,

On Thu, Jan 09, 2020 at 11:04:01PM +0530, Vidya Sagar wrote:
> 
> 
> On 1/9/2020 11:36 AM, Shawn Guo wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Some platform has 4 (or more) viewports.  In that case, CFG0 and CFG1
> > can be separated into different ATU regions.
> Is there any specific benefit with this scheme?

Thanks much for the question which leads me to go back to vendor for
checking design details of 4 (or more) viewports.

It turns out the patch is not complete.  We need more code change to
get the benefit of using separate ATU region for CFG0 and CFG1, that
is the dw_pcie_prog_outbound_atu() call in dw_pcie_access_other_conf()
function can  be saved.  But in the meanwhile, we need to pass
'va_cfg_base | busdev' as the first argument to dw_pcie_write/read()
in there.

@Lorenzo, @Bjorn,

Please ignore this patch.

Shawn

Patch
diff mbox series

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