| Message ID | 20200109060657.1953-1-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: dwc: Separate CFG0 and CFG1 into different ATU regions | expand |
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>
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
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.
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
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 >
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
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
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
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(-)