| Message ID | 1415682444-24911-1-git-send-email-Minghuan.Lian@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian <Minghuan.Lian@freescale.com> wrote: > Currently, pcie-designware.c only supports two ATUs, ATU0 is used > for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict > when MEM and CFG0 are accessed simultaneously. The patch adds > 'num-atus' property to PCIe dts node to describe the number of > PCIe controller's ATUs. If num_atus is bigger than or equal to 4, > we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, > ATU2 for MEM, ATU3 for IO. > > Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> > --- > change log: > v1-v2: > 1. add the default value to property num-atus description > 2. Use atu_idx[] instead of single values > 3. initialize num_atus to 2 > > .../devicetree/bindings/pci/designware-pcie.txt | 1 + > drivers/pci/host/pcie-designware.c | 53 ++++++++++++++++------ > drivers/pci/host/pcie-designware.h | 9 ++++ > 3 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index 9f4faa8..64d0533 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -26,3 +26,4 @@ Optional properties: > - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to > specify this property, to keep backwards compatibility a range of 0x00-0xff > is assumed if not present) > +- num-atus: number of ATUs. The default value is 2 if not present. > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index dfed00a..46a609d 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -48,6 +48,8 @@ > #define PCIE_ATU_VIEWPORT 0x900 > #define PCIE_ATU_REGION_INBOUND (0x1 << 31) > #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) > +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) > +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) > #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) > #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) > #define PCIE_ATU_CR1 0x904 > @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > struct of_pci_range range; > struct of_pci_range_parser parser; > struct resource *cfg_res; > - u32 val, na, ns; > + u32 num_atus = 2, val, na, ns; I think this can be u8? > const __be32 *addrp; > int i, index, ret; > > @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > } > } > > + of_property_read_u32(np, "num-atus", &num_atus); and here too? > + if (num_atus >= 4) { > + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; > + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; > + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; > + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; > + } else { > + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; > + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; > + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; > + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; > + } > + > if (pp->ops->host_init) > pp->ops->host_init(pp); > > @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > > static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) > { > - /* Program viewport 0 : OUTBOUND : CFG0 */ > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, > + /* Program viewport : OUTBOUND : CFG0 */ > + dw_pcie_writel_rc(pp, > + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG0], > PCIE_ATU_VIEWPORT); > dw_pcie_writel_rc(pp, pp->cfg0_mod_base, PCIE_ATU_LOWER_BASE); > dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), PCIE_ATU_UPPER_BASE); > @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) > > static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) > { > - /* Program viewport 1 : OUTBOUND : CFG1 */ > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, > + /* Program viewport : OUTBOUND : CFG1 */ > + dw_pcie_writel_rc(pp, > + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG1], > PCIE_ATU_VIEWPORT); > dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); > dw_pcie_writel_rc(pp, pp->cfg1_mod_base, PCIE_ATU_LOWER_BASE); > @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) > > static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) > { > - /* Program viewport 0 : OUTBOUND : MEM */ > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, > + /* Program viewport : OUTBOUND : MEM */ > + dw_pcie_writel_rc(pp, > + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_MEM], > PCIE_ATU_VIEWPORT); > dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); > dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); > @@ -557,8 +575,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) > > static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) > { > - /* Program viewport 1 : OUTBOUND : IO */ > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, > + /* Program viewport : OUTBOUND : IO */ > + dw_pcie_writel_rc(pp, > + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_IO], > PCIE_ATU_VIEWPORT); > dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); > dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); > @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, > dw_pcie_prog_viewport_cfg0(pp, busdev); > ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, where, size, > val); > - dw_pcie_prog_viewport_mem_outbound(pp); > + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) > + dw_pcie_prog_viewport_mem_outbound(pp); > } else { > dw_pcie_prog_viewport_cfg1(pp, busdev); > ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, where, size, > val); > - dw_pcie_prog_viewport_io_outbound(pp); > + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) > + dw_pcie_prog_viewport_io_outbound(pp); > } > > return ret; > @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, > dw_pcie_prog_viewport_cfg0(pp, busdev); > ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, where, size, > val); > - dw_pcie_prog_viewport_mem_outbound(pp); > + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) > + dw_pcie_prog_viewport_mem_outbound(pp); > } else { > dw_pcie_prog_viewport_cfg1(pp, busdev); > ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, where, size, > val); > - dw_pcie_prog_viewport_io_outbound(pp); > + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) > + dw_pcie_prog_viewport_io_outbound(pp); > } > > return ret; > @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > u32 membase; > u32 memlimit; > > + /* set ATUs setting for MEM and IO */ > + dw_pcie_prog_viewport_mem_outbound(pp); > + dw_pcie_prog_viewport_io_outbound(pp); > + I see from the code, these settings are required for the calls other than dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this change is independent of this patch and should go as a separte patch? - Srikanth > /* set the number of lanes */ > dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); > val &= ~PORT_LINK_MODE_MASK; > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index c625675..37604f9 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -22,6 +22,14 @@ > #define MAX_MSI_IRQS 32 > #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) > > +enum ATU_TYPE { > + ATU_TYPE_CFG0, > + ATU_TYPE_CFG1, > + ATU_TYPE_MEM, > + ATU_TYPE_IO, > + ATU_TYPE_MAX > +}; > + > struct pcie_port { > struct device *dev; > u8 root_bus_nr; > @@ -53,6 +61,7 @@ struct pcie_port { > struct irq_domain *irq_domain; > unsigned long msi_data; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > + u8 atu_idx[ATU_TYPE_MAX]; > }; > > struct pcie_host_ops { > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Srikanth, Thanks for your comments, please see my reply inline. On 2014?11?12? 14:22, Srikanth Thokala wrote: > Hi, > > On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian > <Minghuan.Lian@freescale.com> wrote: >> Currently, pcie-designware.c only supports two ATUs, ATU0 is used >> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict >> when MEM and CFG0 are accessed simultaneously. The patch adds >> 'num-atus' property to PCIe dts node to describe the number of >> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, >> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, >> ATU2 for MEM, ATU3 for IO. >> >> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> >> --- >> change log: >> v1-v2: >> 1. add the default value to property num-atus description >> 2. Use atu_idx[] instead of single values >> 3. initialize num_atus to 2 >> >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >> drivers/pci/host/pcie-designware.c | 53 ++++++++++++++++------ >> drivers/pci/host/pcie-designware.h | 9 ++++ >> 3 files changed, 50 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> index 9f4faa8..64d0533 100644 >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> @@ -26,3 +26,4 @@ Optional properties: >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to >> specify this property, to keep backwards compatibility a range of 0x00-0xff >> is assumed if not present) >> +- num-atus: number of ATUs. The default value is 2 if not present. >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >> index dfed00a..46a609d 100644 >> --- a/drivers/pci/host/pcie-designware.c >> +++ b/drivers/pci/host/pcie-designware.c >> @@ -48,6 +48,8 @@ >> #define PCIE_ATU_VIEWPORT 0x900 >> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) >> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >> #define PCIE_ATU_CR1 0x904 >> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> struct of_pci_range range; >> struct of_pci_range_parser parser; >> struct resource *cfg_res; >> - u32 val, na, ns; >> + u32 num_atus = 2, val, na, ns; > I think this can be u8? [Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs) So, num_atus should be u32 type. If we use u8 type to define num_atus, the dts node should be changed to num_atus = /bits/ 8 <8>. I prefer the previous definition num-atus = <6> which is more simple and clear. >> const __be32 *addrp; >> int i, index, ret; >> >> @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> } >> } >> >> + of_property_read_u32(np, "num-atus", &num_atus); > and here too? [Minghuan] please refer to the above interpretation. >> + if (num_atus >= 4) { >> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; >> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; >> + } else { >> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; >> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; >> + } >> + >> if (pp->ops->host_init) >> pp->ops->host_init(pp); >> >> @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> >> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) >> { >> - /* Program viewport 0 : OUTBOUND : CFG0 */ >> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, >> + /* Program viewport : OUTBOUND : CFG0 */ >> + dw_pcie_writel_rc(pp, >> + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG0], >> PCIE_ATU_VIEWPORT); >> dw_pcie_writel_rc(pp, pp->cfg0_mod_base, PCIE_ATU_LOWER_BASE); >> dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), PCIE_ATU_UPPER_BASE); >> @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) >> >> static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) >> { >> - /* Program viewport 1 : OUTBOUND : CFG1 */ >> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, >> + /* Program viewport : OUTBOUND : CFG1 */ >> + dw_pcie_writel_rc(pp, >> + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG1], >> PCIE_ATU_VIEWPORT); >> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >> dw_pcie_writel_rc(pp, pp->cfg1_mod_base, PCIE_ATU_LOWER_BASE); >> @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) >> >> static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >> { >> - /* Program viewport 0 : OUTBOUND : MEM */ >> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, >> + /* Program viewport : OUTBOUND : MEM */ >> + dw_pcie_writel_rc(pp, >> + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_MEM], >> PCIE_ATU_VIEWPORT); >> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); >> dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); >> @@ -557,8 +575,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >> >> static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) >> { >> - /* Program viewport 1 : OUTBOUND : IO */ >> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, >> + /* Program viewport : OUTBOUND : IO */ >> + dw_pcie_writel_rc(pp, >> + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_IO], >> PCIE_ATU_VIEWPORT); >> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); >> dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); >> @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, >> dw_pcie_prog_viewport_cfg0(pp, busdev); >> ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, where, size, >> val); >> - dw_pcie_prog_viewport_mem_outbound(pp); >> + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) >> + dw_pcie_prog_viewport_mem_outbound(pp); >> } else { >> dw_pcie_prog_viewport_cfg1(pp, busdev); >> ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, where, size, >> val); >> - dw_pcie_prog_viewport_io_outbound(pp); >> + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) >> + dw_pcie_prog_viewport_io_outbound(pp); >> } >> >> return ret; >> @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, >> dw_pcie_prog_viewport_cfg0(pp, busdev); >> ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, where, size, >> val); >> - dw_pcie_prog_viewport_mem_outbound(pp); >> + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) >> + dw_pcie_prog_viewport_mem_outbound(pp); >> } else { >> dw_pcie_prog_viewport_cfg1(pp, busdev); >> ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, where, size, >> val); >> - dw_pcie_prog_viewport_io_outbound(pp); >> + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) >> + dw_pcie_prog_viewport_io_outbound(pp); >> } >> >> return ret; >> @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >> u32 membase; >> u32 memlimit; >> >> + /* set ATUs setting for MEM and IO */ >> + dw_pcie_prog_viewport_mem_outbound(pp); >> + dw_pcie_prog_viewport_io_outbound(pp); >> + > I see from the code, these settings are required for the calls other than > dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this change is > independent of this patch and should go as a separte patch? [Minghuan] dw_pcie(rd/wr)_other_confg only calls the dw_pcie_prog_viewport_mem/io_outbound when we only use 2 ATUs. The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will not be initialized, and PCIe device driver will be broken. > - Srikanth > >> /* set the number of lanes */ >> dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); >> val &= ~PORT_LINK_MODE_MASK; >> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h >> index c625675..37604f9 100644 >> --- a/drivers/pci/host/pcie-designware.h >> +++ b/drivers/pci/host/pcie-designware.h >> @@ -22,6 +22,14 @@ >> #define MAX_MSI_IRQS 32 >> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) >> >> +enum ATU_TYPE { >> + ATU_TYPE_CFG0, >> + ATU_TYPE_CFG1, >> + ATU_TYPE_MEM, >> + ATU_TYPE_IO, >> + ATU_TYPE_MAX >> +}; >> + >> struct pcie_port { >> struct device *dev; >> u8 root_bus_nr; >> @@ -53,6 +61,7 @@ struct pcie_port { >> struct irq_domain *irq_domain; >> unsigned long msi_data; >> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >> + u8 atu_idx[ATU_TYPE_MAX]; >> }; >> >> struct pcie_host_ops { >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Minghuan, On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <B31939@freescale.com> wrote: > Hi Srikanth, > > Thanks for your comments, please see my reply inline. > > > On 2014?11?12? 14:22, Srikanth Thokala wrote: >> >> Hi, >> >> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian >> <Minghuan.Lian@freescale.com> wrote: >>> >>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used >>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict >>> when MEM and CFG0 are accessed simultaneously. The patch adds >>> 'num-atus' property to PCIe dts node to describe the number of >>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, >>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, >>> ATU2 for MEM, ATU3 for IO. >>> >>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> >>> --- >>> change log: >>> v1-v2: >>> 1. add the default value to property num-atus description >>> 2. Use atu_idx[] instead of single values >>> 3. initialize num_atus to 2 >>> >>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>> drivers/pci/host/pcie-designware.c | 53 >>> ++++++++++++++++------ >>> drivers/pci/host/pcie-designware.h | 9 ++++ >>> 3 files changed, 50 insertions(+), 13 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> index 9f4faa8..64d0533 100644 >>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> @@ -26,3 +26,4 @@ Optional properties: >>> - bus-range: PCI bus numbers covered (it is recommended for new >>> devicetrees to >>> specify this property, to keep backwards compatibility a range of >>> 0x00-0xff >>> is assumed if not present) >>> +- num-atus: number of ATUs. The default value is 2 if not present. >>> diff --git a/drivers/pci/host/pcie-designware.c >>> b/drivers/pci/host/pcie-designware.c >>> index dfed00a..46a609d 100644 >>> --- a/drivers/pci/host/pcie-designware.c >>> +++ b/drivers/pci/host/pcie-designware.c >>> @@ -48,6 +48,8 @@ >>> #define PCIE_ATU_VIEWPORT 0x900 >>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >>> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) >>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >>> #define PCIE_ATU_CR1 0x904 >>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>> struct of_pci_range range; >>> struct of_pci_range_parser parser; >>> struct resource *cfg_res; >>> - u32 val, na, ns; >>> + u32 num_atus = 2, val, na, ns; >> >> I think this can be u8? > > [Minghuan] I define num-atus like this: num-atus = <6> (our controller > supports 6 outbound ATUs) > So, num_atus should be u32 type. > If we use u8 type to define num_atus, the dts node should be changed to > num_atus = /bits/ 8 <8>. > I prefer the previous definition num-atus = <6> which is more simple and > clear. Yes, I agree. But as it holds only 6 possible distinct values, I prefer to use u8. >>> >>> const __be32 *addrp; >>> int i, index, ret; >>> >>> @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>> } >>> } >>> >>> + of_property_read_u32(np, "num-atus", &num_atus); >> >> and here too? > > [Minghuan] please refer to the above interpretation. > >>> + if (num_atus >= 4) { >>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; >>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; >>> + } else { >>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; >>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; >>> + } >>> + >>> if (pp->ops->host_init) >>> pp->ops->host_init(pp); >>> >>> @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>> >>> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 >>> busdev) >>> { >>> - /* Program viewport 0 : OUTBOUND : CFG0 */ >>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>> PCIE_ATU_REGION_INDEX0, >>> + /* Program viewport : OUTBOUND : CFG0 */ >>> + dw_pcie_writel_rc(pp, >>> + PCIE_ATU_REGION_OUTBOUND | >>> pp->atu_idx[ATU_TYPE_CFG0], >>> PCIE_ATU_VIEWPORT); >>> dw_pcie_writel_rc(pp, pp->cfg0_mod_base, PCIE_ATU_LOWER_BASE); >>> dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), >>> PCIE_ATU_UPPER_BASE); >>> @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct >>> pcie_port *pp, u32 busdev) >>> >>> static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 >>> busdev) >>> { >>> - /* Program viewport 1 : OUTBOUND : CFG1 */ >>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>> PCIE_ATU_REGION_INDEX1, >>> + /* Program viewport : OUTBOUND : CFG1 */ >>> + dw_pcie_writel_rc(pp, >>> + PCIE_ATU_REGION_OUTBOUND | >>> pp->atu_idx[ATU_TYPE_CFG1], >>> PCIE_ATU_VIEWPORT); >>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >>> dw_pcie_writel_rc(pp, pp->cfg1_mod_base, PCIE_ATU_LOWER_BASE); >>> @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct >>> pcie_port *pp, u32 busdev) >>> >>> static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >>> { >>> - /* Program viewport 0 : OUTBOUND : MEM */ >>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>> PCIE_ATU_REGION_INDEX0, >>> + /* Program viewport : OUTBOUND : MEM */ >>> + dw_pcie_writel_rc(pp, >>> + PCIE_ATU_REGION_OUTBOUND | >>> pp->atu_idx[ATU_TYPE_MEM], >>> PCIE_ATU_VIEWPORT); >>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); >>> dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); >>> @@ -557,8 +575,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct >>> pcie_port *pp) >>> >>> static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) >>> { >>> - /* Program viewport 1 : OUTBOUND : IO */ >>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>> PCIE_ATU_REGION_INDEX1, >>> + /* Program viewport : OUTBOUND : IO */ >>> + dw_pcie_writel_rc(pp, >>> + PCIE_ATU_REGION_OUTBOUND | >>> pp->atu_idx[ATU_TYPE_IO], >>> PCIE_ATU_VIEWPORT); >>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); >>> dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); >>> @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port >>> *pp, struct pci_bus *bus, >>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>> ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, >>> where, size, >>> val); >>> - dw_pcie_prog_viewport_mem_outbound(pp); >>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>> pp->atu_idx[ATU_TYPE_CFG0]) >>> + dw_pcie_prog_viewport_mem_outbound(pp); >>> } else { >>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>> ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, >>> where, size, >>> val); >>> - dw_pcie_prog_viewport_io_outbound(pp); >>> + if (pp->atu_idx[ATU_TYPE_IO] == >>> pp->atu_idx[ATU_TYPE_CFG1]) >>> + dw_pcie_prog_viewport_io_outbound(pp); >>> } >>> >>> return ret; >>> @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port >>> *pp, struct pci_bus *bus, >>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>> ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, >>> where, size, >>> val); >>> - dw_pcie_prog_viewport_mem_outbound(pp); >>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>> pp->atu_idx[ATU_TYPE_CFG0]) >>> + dw_pcie_prog_viewport_mem_outbound(pp); >>> } else { >>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>> ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, >>> where, size, >>> val); >>> - dw_pcie_prog_viewport_io_outbound(pp); >>> + if (pp->atu_idx[ATU_TYPE_IO] == >>> pp->atu_idx[ATU_TYPE_CFG1]) >>> + dw_pcie_prog_viewport_io_outbound(pp); >>> } >>> >>> return ret; >>> @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>> u32 membase; >>> u32 memlimit; >>> >>> + /* set ATUs setting for MEM and IO */ >>> + dw_pcie_prog_viewport_mem_outbound(pp); >>> + dw_pcie_prog_viewport_io_outbound(pp); >>> + >> >> I see from the code, these settings are required for the calls other than >> dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this change >> is >> independent of this patch and should go as a separte patch? > > [Minghuan] dw_pcie(rd/wr)_other_confg only calls the > dw_pcie_prog_viewport_mem/io_outbound when > we only use 2 ATUs. > The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will > not be initialized, and PCIe device driver will be broken. When you have only 2 ATUs (CFG0=MEM & CFG1=IO), you are calling mem/io_outbound() twice with the same writes which is not the case in the original driver. So, I mentioned it should go as a separate patch. - Srikanth > >> - Srikanth >> >>> /* set the number of lanes */ >>> dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); >>> val &= ~PORT_LINK_MODE_MASK; >>> diff --git a/drivers/pci/host/pcie-designware.h >>> b/drivers/pci/host/pcie-designware.h >>> index c625675..37604f9 100644 >>> --- a/drivers/pci/host/pcie-designware.h >>> +++ b/drivers/pci/host/pcie-designware.h >>> @@ -22,6 +22,14 @@ >>> #define MAX_MSI_IRQS 32 >>> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) >>> >>> +enum ATU_TYPE { >>> + ATU_TYPE_CFG0, >>> + ATU_TYPE_CFG1, >>> + ATU_TYPE_MEM, >>> + ATU_TYPE_IO, >>> + ATU_TYPE_MAX >>> +}; >>> + >>> struct pcie_port { >>> struct device *dev; >>> u8 root_bus_nr; >>> @@ -53,6 +61,7 @@ struct pcie_port { >>> struct irq_domain *irq_domain; >>> unsigned long msi_data; >>> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >>> + u8 atu_idx[ATU_TYPE_MAX]; >>> }; >>> >>> struct pcie_host_ops { >>> -- >>> 1.9.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hi Srikanth, please see my comments inline. Thanks, Minghuan On 2014?11?12? 17:01, Srikanth Thokala wrote: > Hi Minghuan, > > On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 > <B31939@freescale.com> wrote: >> Hi Srikanth, >> >> Thanks for your comments, please see my reply inline. >> >> >> On 2014?11?12? 14:22, Srikanth Thokala wrote: >>> Hi, >>> >>> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian >>> <Minghuan.Lian@freescale.com> wrote: >>>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used >>>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict >>>> when MEM and CFG0 are accessed simultaneously. The patch adds >>>> 'num-atus' property to PCIe dts node to describe the number of >>>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, >>>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, >>>> ATU2 for MEM, ATU3 for IO. >>>> >>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> >>>> --- >>>> change log: >>>> v1-v2: >>>> 1. add the default value to property num-atus description >>>> 2. Use atu_idx[] instead of single values >>>> 3. initialize num_atus to 2 >>>> >>>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>>> drivers/pci/host/pcie-designware.c | 53 >>>> ++++++++++++++++------ >>>> drivers/pci/host/pcie-designware.h | 9 ++++ >>>> 3 files changed, 50 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>> index 9f4faa8..64d0533 100644 >>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>> @@ -26,3 +26,4 @@ Optional properties: >>>> - bus-range: PCI bus numbers covered (it is recommended for new >>>> devicetrees to >>>> specify this property, to keep backwards compatibility a range of >>>> 0x00-0xff >>>> is assumed if not present) >>>> +- num-atus: number of ATUs. The default value is 2 if not present. >>>> diff --git a/drivers/pci/host/pcie-designware.c >>>> b/drivers/pci/host/pcie-designware.c >>>> index dfed00a..46a609d 100644 >>>> --- a/drivers/pci/host/pcie-designware.c >>>> +++ b/drivers/pci/host/pcie-designware.c >>>> @@ -48,6 +48,8 @@ >>>> #define PCIE_ATU_VIEWPORT 0x900 >>>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >>>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >>>> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) >>>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >>>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >>>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >>>> #define PCIE_ATU_CR1 0x904 >>>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>> struct of_pci_range range; >>>> struct of_pci_range_parser parser; >>>> struct resource *cfg_res; >>>> - u32 val, na, ns; >>>> + u32 num_atus = 2, val, na, ns; >>> I think this can be u8? >> [Minghuan] I define num-atus like this: num-atus = <6> (our controller >> supports 6 outbound ATUs) >> So, num_atus should be u32 type. >> If we use u8 type to define num_atus, the dts node should be changed to >> num_atus = /bits/ 8 <8>. >> I prefer the previous definition num-atus = <6> which is more simple and >> clear. > Yes, I agree. But as it holds only 6 possible distinct values, I > prefer to use u8. [Minghuan] PCIe Designware IP supports more than 6 ATUs. But our current PCIe controller only supports 6 outbound ATUs and 6 inbound ATUs. The next PCIe controller may supports more ATUs. I think u32 can be better compatible with hardware upgrade. I inquired dts, almost all dts property use u32 type. for example: #address-cells = <3>; #size-cells = <2>; num-lanes = <1>; >>>> const __be32 *addrp; >>>> int i, index, ret; >>>> >>>> @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>> } >>>> } >>>> >>>> + of_property_read_u32(np, "num-atus", &num_atus); >>> and here too? >> [Minghuan] please refer to the above interpretation. >> >>>> + if (num_atus >= 4) { >>>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; >>>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; >>>> + } else { >>>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; >>>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; >>>> + } >>>> + >>>> if (pp->ops->host_init) >>>> pp->ops->host_init(pp); >>>> >>>> @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>> >>>> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 >>>> busdev) >>>> { >>>> - /* Program viewport 0 : OUTBOUND : CFG0 */ >>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>> PCIE_ATU_REGION_INDEX0, >>>> + /* Program viewport : OUTBOUND : CFG0 */ >>>> + dw_pcie_writel_rc(pp, >>>> + PCIE_ATU_REGION_OUTBOUND | >>>> pp->atu_idx[ATU_TYPE_CFG0], >>>> PCIE_ATU_VIEWPORT); >>>> dw_pcie_writel_rc(pp, pp->cfg0_mod_base, PCIE_ATU_LOWER_BASE); >>>> dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), >>>> PCIE_ATU_UPPER_BASE); >>>> @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct >>>> pcie_port *pp, u32 busdev) >>>> >>>> static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 >>>> busdev) >>>> { >>>> - /* Program viewport 1 : OUTBOUND : CFG1 */ >>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>> PCIE_ATU_REGION_INDEX1, >>>> + /* Program viewport : OUTBOUND : CFG1 */ >>>> + dw_pcie_writel_rc(pp, >>>> + PCIE_ATU_REGION_OUTBOUND | >>>> pp->atu_idx[ATU_TYPE_CFG1], >>>> PCIE_ATU_VIEWPORT); >>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >>>> dw_pcie_writel_rc(pp, pp->cfg1_mod_base, PCIE_ATU_LOWER_BASE); >>>> @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct >>>> pcie_port *pp, u32 busdev) >>>> >>>> static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >>>> { >>>> - /* Program viewport 0 : OUTBOUND : MEM */ >>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>> PCIE_ATU_REGION_INDEX0, >>>> + /* Program viewport : OUTBOUND : MEM */ >>>> + dw_pcie_writel_rc(pp, >>>> + PCIE_ATU_REGION_OUTBOUND | >>>> pp->atu_idx[ATU_TYPE_MEM], >>>> PCIE_ATU_VIEWPORT); >>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); >>>> dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); >>>> @@ -557,8 +575,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct >>>> pcie_port *pp) >>>> >>>> static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) >>>> { >>>> - /* Program viewport 1 : OUTBOUND : IO */ >>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>> PCIE_ATU_REGION_INDEX1, >>>> + /* Program viewport : OUTBOUND : IO */ >>>> + dw_pcie_writel_rc(pp, >>>> + PCIE_ATU_REGION_OUTBOUND | >>>> pp->atu_idx[ATU_TYPE_IO], >>>> PCIE_ATU_VIEWPORT); >>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); >>>> dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); >>>> @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port >>>> *pp, struct pci_bus *bus, >>>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>>> ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, >>>> where, size, >>>> val); >>>> - dw_pcie_prog_viewport_mem_outbound(pp); >>>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>>> pp->atu_idx[ATU_TYPE_CFG0]) >>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>> } else { >>>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>>> ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, >>>> where, size, >>>> val); >>>> - dw_pcie_prog_viewport_io_outbound(pp); >>>> + if (pp->atu_idx[ATU_TYPE_IO] == >>>> pp->atu_idx[ATU_TYPE_CFG1]) >>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>> } >>>> >>>> return ret; >>>> @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port >>>> *pp, struct pci_bus *bus, >>>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>>> ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, >>>> where, size, >>>> val); >>>> - dw_pcie_prog_viewport_mem_outbound(pp); >>>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>>> pp->atu_idx[ATU_TYPE_CFG0]) >>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>> } else { >>>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>>> ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, >>>> where, size, >>>> val); >>>> - dw_pcie_prog_viewport_io_outbound(pp); >>>> + if (pp->atu_idx[ATU_TYPE_IO] == >>>> pp->atu_idx[ATU_TYPE_CFG1]) >>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>> } >>>> >>>> return ret; >>>> @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>>> u32 membase; >>>> u32 memlimit; >>>> >>>> + /* set ATUs setting for MEM and IO */ >>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>> + >>> I see from the code, these settings are required for the calls other than >>> dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this change >>> is >>> independent of this patch and should go as a separte patch? >> [Minghuan] dw_pcie(rd/wr)_other_confg only calls the >> dw_pcie_prog_viewport_mem/io_outbound when >> we only use 2 ATUs. >> The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will >> not be initialized, and PCIe device driver will be broken. > When you have only 2 ATUs (CFG0=MEM & CFG1=IO), you are calling > mem/io_outbound() twice with the same writes which is not the case in the > original driver. So, I mentioned it should go as a separate patch. [Minghuan] Sorry, I do not understand what you mean. How to separate patch? A patch is to add the following code based on original code + /* set ATUs setting for MEM and IO */ + dw_pcie_prog_viewport_mem_outbound(pp); + dw_pcie_prog_viewport_io_outbound(pp); + But why add this patch? 2 ATUs does not need them. Only 4 ATUs support needs them. And them are also ok for 2 ATUs. For 2 ATUs, mem/io will be initialized every time read/write PCI device configuration. - Srikanth >>> - Srikanth >>> >>>> /* set the number of lanes */ >>>> dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); >>>> val &= ~PORT_LINK_MODE_MASK; >>>> diff --git a/drivers/pci/host/pcie-designware.h >>>> b/drivers/pci/host/pcie-designware.h >>>> index c625675..37604f9 100644 >>>> --- a/drivers/pci/host/pcie-designware.h >>>> +++ b/drivers/pci/host/pcie-designware.h >>>> @@ -22,6 +22,14 @@ >>>> #define MAX_MSI_IRQS 32 >>>> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) >>>> >>>> +enum ATU_TYPE { >>>> + ATU_TYPE_CFG0, >>>> + ATU_TYPE_CFG1, >>>> + ATU_TYPE_MEM, >>>> + ATU_TYPE_IO, >>>> + ATU_TYPE_MAX >>>> +}; >>>> + >>>> struct pcie_port { >>>> struct device *dev; >>>> u8 root_bus_nr; >>>> @@ -53,6 +61,7 @@ struct pcie_port { >>>> struct irq_domain *irq_domain; >>>> unsigned long msi_data; >>>> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >>>> + u8 atu_idx[ATU_TYPE_MAX]; >>>> }; >>>> >>>> struct pcie_host_ops { >>>> -- >>>> 1.9.1 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
Hi Minghuan, On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 <B31939@freescale.com> wrote: > Hi Srikanth, > > please see my comments inline. > > Thanks, > Minghuan > > > On 2014?11?12? 17:01, Srikanth Thokala wrote: >> >> Hi Minghuan, >> >> On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 >> <B31939@freescale.com> wrote: >>> >>> Hi Srikanth, >>> >>> Thanks for your comments, please see my reply inline. >>> >>> >>> On 2014?11?12? 14:22, Srikanth Thokala wrote: >>>> >>>> Hi, >>>> >>>> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian >>>> <Minghuan.Lian@freescale.com> wrote: >>>>> >>>>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used >>>>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict >>>>> when MEM and CFG0 are accessed simultaneously. The patch adds >>>>> 'num-atus' property to PCIe dts node to describe the number of >>>>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, >>>>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, >>>>> ATU2 for MEM, ATU3 for IO. >>>>> >>>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> >>>>> --- >>>>> change log: >>>>> v1-v2: >>>>> 1. add the default value to property num-atus description >>>>> 2. Use atu_idx[] instead of single values >>>>> 3. initialize num_atus to 2 >>>>> >>>>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>>>> drivers/pci/host/pcie-designware.c | 53 >>>>> ++++++++++++++++------ >>>>> drivers/pci/host/pcie-designware.h | 9 ++++ >>>>> 3 files changed, 50 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>> index 9f4faa8..64d0533 100644 >>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>> @@ -26,3 +26,4 @@ Optional properties: >>>>> - bus-range: PCI bus numbers covered (it is recommended for new >>>>> devicetrees to >>>>> specify this property, to keep backwards compatibility a range of >>>>> 0x00-0xff >>>>> is assumed if not present) >>>>> +- num-atus: number of ATUs. The default value is 2 if not present. >>>>> diff --git a/drivers/pci/host/pcie-designware.c >>>>> b/drivers/pci/host/pcie-designware.c >>>>> index dfed00a..46a609d 100644 >>>>> --- a/drivers/pci/host/pcie-designware.c >>>>> +++ b/drivers/pci/host/pcie-designware.c >>>>> @@ -48,6 +48,8 @@ >>>>> #define PCIE_ATU_VIEWPORT 0x900 >>>>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >>>>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >>>>> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) >>>>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >>>>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >>>>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >>>>> #define PCIE_ATU_CR1 0x904 >>>>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>>> struct of_pci_range range; >>>>> struct of_pci_range_parser parser; >>>>> struct resource *cfg_res; >>>>> - u32 val, na, ns; >>>>> + u32 num_atus = 2, val, na, ns; >>>> >>>> I think this can be u8? >>> >>> [Minghuan] I define num-atus like this: num-atus = <6> (our controller >>> supports 6 outbound ATUs) >>> So, num_atus should be u32 type. >>> If we use u8 type to define num_atus, the dts node should be changed to >>> num_atus = /bits/ 8 <8>. >>> I prefer the previous definition num-atus = <6> which is more simple and >>> clear. >> >> Yes, I agree. But as it holds only 6 possible distinct values, I >> prefer to use u8. > > [Minghuan] PCIe Designware IP supports more than 6 ATUs. But our > current PCIe controller only supports 6 outbound ATUs and 6 inbound ATUs. > The next PCIe controller may supports more ATUs. I think u32 can be better > compatible with hardware upgrade. > > I inquired dts, almost all dts property use u32 type. I don't think this property will have values > 255, but if you think so you could use u16 and then u32. > for example: > #address-cells = <3>; > #size-cells = <2>; > num-lanes = <1>; > > >>>>> const __be32 *addrp; >>>>> int i, index, ret; >>>>> >>>>> @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>>> } >>>>> } >>>>> >>>>> + of_property_read_u32(np, "num-atus", &num_atus); >>>> >>>> and here too? >>> >>> [Minghuan] please refer to the above interpretation. >>> >>>>> + if (num_atus >= 4) { >>>>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>>>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>>>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; >>>>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; >>>>> + } else { >>>>> + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; >>>>> + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; >>>>> + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; >>>>> + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; >>>>> + } >>>>> + >>>>> if (pp->ops->host_init) >>>>> pp->ops->host_init(pp); >>>>> >>>>> @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>>> >>>>> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 >>>>> busdev) >>>>> { >>>>> - /* Program viewport 0 : OUTBOUND : CFG0 */ >>>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>>> PCIE_ATU_REGION_INDEX0, >>>>> + /* Program viewport : OUTBOUND : CFG0 */ >>>>> + dw_pcie_writel_rc(pp, >>>>> + PCIE_ATU_REGION_OUTBOUND | >>>>> pp->atu_idx[ATU_TYPE_CFG0], >>>>> PCIE_ATU_VIEWPORT); >>>>> dw_pcie_writel_rc(pp, pp->cfg0_mod_base, >>>>> PCIE_ATU_LOWER_BASE); >>>>> dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), >>>>> PCIE_ATU_UPPER_BASE); >>>>> @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct >>>>> pcie_port *pp, u32 busdev) >>>>> >>>>> static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 >>>>> busdev) >>>>> { >>>>> - /* Program viewport 1 : OUTBOUND : CFG1 */ >>>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>>> PCIE_ATU_REGION_INDEX1, >>>>> + /* Program viewport : OUTBOUND : CFG1 */ >>>>> + dw_pcie_writel_rc(pp, >>>>> + PCIE_ATU_REGION_OUTBOUND | >>>>> pp->atu_idx[ATU_TYPE_CFG1], >>>>> PCIE_ATU_VIEWPORT); >>>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >>>>> dw_pcie_writel_rc(pp, pp->cfg1_mod_base, >>>>> PCIE_ATU_LOWER_BASE); >>>>> @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct >>>>> pcie_port *pp, u32 busdev) >>>>> >>>>> static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >>>>> { >>>>> - /* Program viewport 0 : OUTBOUND : MEM */ >>>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>>> PCIE_ATU_REGION_INDEX0, >>>>> + /* Program viewport : OUTBOUND : MEM */ >>>>> + dw_pcie_writel_rc(pp, >>>>> + PCIE_ATU_REGION_OUTBOUND | >>>>> pp->atu_idx[ATU_TYPE_MEM], >>>>> PCIE_ATU_VIEWPORT); >>>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); >>>>> dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); >>>>> @@ -557,8 +575,9 @@ static void >>>>> dw_pcie_prog_viewport_mem_outbound(struct >>>>> pcie_port *pp) >>>>> >>>>> static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) >>>>> { >>>>> - /* Program viewport 1 : OUTBOUND : IO */ >>>>> - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >>>>> PCIE_ATU_REGION_INDEX1, >>>>> + /* Program viewport : OUTBOUND : IO */ >>>>> + dw_pcie_writel_rc(pp, >>>>> + PCIE_ATU_REGION_OUTBOUND | >>>>> pp->atu_idx[ATU_TYPE_IO], >>>>> PCIE_ATU_VIEWPORT); >>>>> dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); >>>>> dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); >>>>> @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port >>>>> *pp, struct pci_bus *bus, >>>>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>>>> ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, >>>>> where, size, >>>>> val); >>>>> - dw_pcie_prog_viewport_mem_outbound(pp); >>>>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>>>> pp->atu_idx[ATU_TYPE_CFG0]) >>>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>>> } else { >>>>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>>>> ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, >>>>> where, size, >>>>> val); >>>>> - dw_pcie_prog_viewport_io_outbound(pp); >>>>> + if (pp->atu_idx[ATU_TYPE_IO] == >>>>> pp->atu_idx[ATU_TYPE_CFG1]) >>>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>>> } >>>>> >>>>> return ret; >>>>> @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port >>>>> *pp, struct pci_bus *bus, >>>>> dw_pcie_prog_viewport_cfg0(pp, busdev); >>>>> ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, >>>>> where, size, >>>>> val); >>>>> - dw_pcie_prog_viewport_mem_outbound(pp); >>>>> + if (pp->atu_idx[ATU_TYPE_MEM] == >>>>> pp->atu_idx[ATU_TYPE_CFG0]) >>>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>>> } else { >>>>> dw_pcie_prog_viewport_cfg1(pp, busdev); >>>>> ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, >>>>> where, size, >>>>> val); >>>>> - dw_pcie_prog_viewport_io_outbound(pp); >>>>> + if (pp->atu_idx[ATU_TYPE_IO] == >>>>> pp->atu_idx[ATU_TYPE_CFG1]) >>>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>>> } >>>>> >>>>> return ret; >>>>> @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>>>> u32 membase; >>>>> u32 memlimit; >>>>> >>>>> + /* set ATUs setting for MEM and IO */ >>>>> + dw_pcie_prog_viewport_mem_outbound(pp); >>>>> + dw_pcie_prog_viewport_io_outbound(pp); >>>>> + >>>> >>>> I see from the code, these settings are required for the calls other >>>> than >>>> dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this >>>> change >>>> is >>>> independent of this patch and should go as a separte patch? >>> >>> [Minghuan] dw_pcie(rd/wr)_other_confg only calls the >>> dw_pcie_prog_viewport_mem/io_outbound when >>> we only use 2 ATUs. >>> The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will >>> not be initialized, and PCIe device driver will be broken. >> >> When you have only 2 ATUs (CFG0=MEM & CFG1=IO), you are calling >> mem/io_outbound() twice with the same writes which is not the case in the >> original driver. So, I mentioned it should go as a separate patch. > > [Minghuan] Sorry, I do not understand what you mean. > How to separate patch? > A patch is to add the following code based on original code > > + /* set ATUs setting for MEM and IO */ > + dw_pcie_prog_viewport_mem_outbound(pp); > + dw_pcie_prog_viewport_io_outbound(pp); > + > > But why add this patch? 2 ATUs does not need them. > > Only 4 ATUs support needs them. Then you may have to add a condition here, num_atus >= 4? > And them are also ok for 2 ATUs. You are just re-writing them anyway, so I don't see a place for them here. So, this fragment should just work, +++ if (num_atus >=4 ) { dw_pcie_prog_viewport_mem_outbound(pp); dw_pcie_prog_viewport_io_outbound(pp); } +++ Is that correct? Am I still missing? > For 2 ATUs, mem/io will be initialized every time read/write PCI device > configuration. Just out of curiosity, is it really required to initialize mem/io everytime there is a config read/write? Would it not work when initialized just once like for the case of 4 ATUs? - Srikanth > > > > - Srikanth > >>>> - Srikanth >>>> >>>>> /* set the number of lanes */ >>>>> dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); >>>>> val &= ~PORT_LINK_MODE_MASK; >>>>> diff --git a/drivers/pci/host/pcie-designware.h >>>>> b/drivers/pci/host/pcie-designware.h >>>>> index c625675..37604f9 100644 >>>>> --- a/drivers/pci/host/pcie-designware.h >>>>> +++ b/drivers/pci/host/pcie-designware.h >>>>> @@ -22,6 +22,14 @@ >>>>> #define MAX_MSI_IRQS 32 >>>>> #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) >>>>> >>>>> +enum ATU_TYPE { >>>>> + ATU_TYPE_CFG0, >>>>> + ATU_TYPE_CFG1, >>>>> + ATU_TYPE_MEM, >>>>> + ATU_TYPE_IO, >>>>> + ATU_TYPE_MAX >>>>> +}; >>>>> + >>>>> struct pcie_port { >>>>> struct device *dev; >>>>> u8 root_bus_nr; >>>>> @@ -53,6 +61,7 @@ struct pcie_port { >>>>> struct irq_domain *irq_domain; >>>>> unsigned long msi_data; >>>>> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >>>>> + u8 atu_idx[ATU_TYPE_MAX]; >>>>> }; >>>>> >>>>> struct pcie_host_ops { >>>>> -- >>>>> 1.9.1 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > Hi Minghuan, > > On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 > <B31939@freescale.com> wrote: > > Hi Srikanth, > > > > please see my comments inline. > > > > Thanks, > > Minghuan > > > > > > On 2014?11?12? 17:01, Srikanth Thokala wrote: > >> > >> Hi Minghuan, > >> > >> On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 > >> <B31939@freescale.com> wrote: > >>> > >>> Hi Srikanth, > >>> > >>> Thanks for your comments, please see my reply inline. > >>> > >>> > >>> On 2014?11?12? 14:22, Srikanth Thokala wrote: > >>>> > >>>> Hi, > >>>> > >>>> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian > >>>> <Minghuan.Lian@freescale.com> wrote: > >>>>> > >>>>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used > >>>>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict > >>>>> when MEM and CFG0 are accessed simultaneously. The patch adds > >>>>> 'num-atus' property to PCIe dts node to describe the number of > >>>>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, > >>>>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, > >>>>> ATU2 for MEM, ATU3 for IO. > >>>>> > >>>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> > >>>>> --- > >>>>> change log: > >>>>> v1-v2: > >>>>> 1. add the default value to property num-atus description > >>>>> 2. Use atu_idx[] instead of single values > >>>>> 3. initialize num_atus to 2 > >>>>> > >>>>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + > >>>>> drivers/pci/host/pcie-designware.c | 53 > >>>>> ++++++++++++++++------ > >>>>> drivers/pci/host/pcie-designware.h | 9 ++++ > >>>>> 3 files changed, 50 insertions(+), 13 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>>>> index 9f4faa8..64d0533 100644 > >>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>>>> @@ -26,3 +26,4 @@ Optional properties: > >>>>> - bus-range: PCI bus numbers covered (it is recommended for new > >>>>> devicetrees to > >>>>> specify this property, to keep backwards compatibility a range of > >>>>> 0x00-0xff > >>>>> is assumed if not present) > >>>>> +- num-atus: number of ATUs. The default value is 2 if not present. > >>>>> diff --git a/drivers/pci/host/pcie-designware.c > >>>>> b/drivers/pci/host/pcie-designware.c > >>>>> index dfed00a..46a609d 100644 > >>>>> --- a/drivers/pci/host/pcie-designware.c > >>>>> +++ b/drivers/pci/host/pcie-designware.c > >>>>> @@ -48,6 +48,8 @@ > >>>>> #define PCIE_ATU_VIEWPORT 0x900 > >>>>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) > >>>>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) > >>>>> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) > >>>>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) > >>>>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) > >>>>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) > >>>>> #define PCIE_ATU_CR1 0x904 > >>>>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > >>>>> struct of_pci_range range; > >>>>> struct of_pci_range_parser parser; > >>>>> struct resource *cfg_res; > >>>>> - u32 val, na, ns; > >>>>> + u32 num_atus = 2, val, na, ns; > >>>> > >>>> I think this can be u8? > >>> > >>> [Minghuan] I define num-atus like this: num-atus = <6> (our controller > >>> supports 6 outbound ATUs) > >>> So, num_atus should be u32 type. > >>> If we use u8 type to define num_atus, the dts node should be changed to > >>> num_atus = /bits/ 8 <8>. > >>> I prefer the previous definition num-atus = <6> which is more simple and > >>> clear. > >> > >> Yes, I agree. But as it holds only 6 possible distinct values, I > >> prefer to use u8. > > > > [Minghuan] PCIe Designware IP supports more than 6 ATUs. But our > > current PCIe controller only supports 6 outbound ATUs and 6 inbound ATUs. > > The next PCIe controller may supports more ATUs. I think u32 can be better > > compatible with hardware upgrade. > > > > I inquired dts, almost all dts property use u32 type. > > I don't think this property will have values > 255, but if you think > so you could > use u16 and then u32. > Using a smaller type complicates the DT for little to no benefit. I think it's ok to use u32 here, which is a common standard for integer values in DT. Though this discussion lead me to the question if we even need to have this property in the DT at all. Isn't this a property that is fixed for a specific silicon implementation of the DW core? In that case we could just infer the number of ATUs from the DT compatible, so this should probably just be added to struct pcie_port and properly initialized by the SoC glue drivers. Regards, Lucas
Hi Lucas, Please see my comments inline. Thanks, Minghuan On 2014?11?13? 00:32, Lucas Stach wrote: > Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: >> Hi Minghuan, >> >> On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 >> <B31939@freescale.com> wrote: >>> Hi Srikanth, >>> >>> please see my comments inline. >>> >>> Thanks, >>> Minghuan >>> >>> >>> On 2014?11?12? 17:01, Srikanth Thokala wrote: >>>> Hi Minghuan, >>>> >>>> On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 >>>> <B31939@freescale.com> wrote: >>>>> Hi Srikanth, >>>>> >>>>> Thanks for your comments, please see my reply inline. >>>>> >>>>> >>>>> On 2014?11?12? 14:22, Srikanth Thokala wrote: >>>>>> Hi, >>>>>> >>>>>> On Tue, Nov 11, 2014 at 10:37 AM, Minghuan Lian >>>>>> <Minghuan.Lian@freescale.com> wrote: >>>>>>> Currently, pcie-designware.c only supports two ATUs, ATU0 is used >>>>>>> for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict >>>>>>> when MEM and CFG0 are accessed simultaneously. The patch adds >>>>>>> 'num-atus' property to PCIe dts node to describe the number of >>>>>>> PCIe controller's ATUs. If num_atus is bigger than or equal to 4, >>>>>>> we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, >>>>>>> ATU2 for MEM, ATU3 for IO. >>>>>>> >>>>>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> >>>>>>> --- >>>>>>> change log: >>>>>>> v1-v2: >>>>>>> 1. add the default value to property num-atus description >>>>>>> 2. Use atu_idx[] instead of single values >>>>>>> 3. initialize num_atus to 2 >>>>>>> >>>>>>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>>>>>> drivers/pci/host/pcie-designware.c | 53 >>>>>>> ++++++++++++++++------ >>>>>>> drivers/pci/host/pcie-designware.h | 9 ++++ >>>>>>> 3 files changed, 50 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>>>> index 9f4faa8..64d0533 100644 >>>>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>>>>>> @@ -26,3 +26,4 @@ Optional properties: >>>>>>> - bus-range: PCI bus numbers covered (it is recommended for new >>>>>>> devicetrees to >>>>>>> specify this property, to keep backwards compatibility a range of >>>>>>> 0x00-0xff >>>>>>> is assumed if not present) >>>>>>> +- num-atus: number of ATUs. The default value is 2 if not present. >>>>>>> diff --git a/drivers/pci/host/pcie-designware.c >>>>>>> b/drivers/pci/host/pcie-designware.c >>>>>>> index dfed00a..46a609d 100644 >>>>>>> --- a/drivers/pci/host/pcie-designware.c >>>>>>> +++ b/drivers/pci/host/pcie-designware.c >>>>>>> @@ -48,6 +48,8 @@ >>>>>>> #define PCIE_ATU_VIEWPORT 0x900 >>>>>>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >>>>>>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >>>>>>> +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) >>>>>>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >>>>>>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >>>>>>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >>>>>>> #define PCIE_ATU_CR1 0x904 >>>>>>> @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >>>>>>> struct of_pci_range range; >>>>>>> struct of_pci_range_parser parser; >>>>>>> struct resource *cfg_res; >>>>>>> - u32 val, na, ns; >>>>>>> + u32 num_atus = 2, val, na, ns; >>>>>> I think this can be u8? >>>>> [Minghuan] I define num-atus like this: num-atus = <6> (our controller >>>>> supports 6 outbound ATUs) >>>>> So, num_atus should be u32 type. >>>>> If we use u8 type to define num_atus, the dts node should be changed to >>>>> num_atus = /bits/ 8 <8>. >>>>> I prefer the previous definition num-atus = <6> which is more simple and >>>>> clear. >>>> Yes, I agree. But as it holds only 6 possible distinct values, I >>>> prefer to use u8. >>> [Minghuan] PCIe Designware IP supports more than 6 ATUs. But our >>> current PCIe controller only supports 6 outbound ATUs and 6 inbound ATUs. >>> The next PCIe controller may supports more ATUs. I think u32 can be better >>> compatible with hardware upgrade. >>> >>> I inquired dts, almost all dts property use u32 type. >> I don't think this property will have values > 255, but if you think >> so you could >> use u16 and then u32. >> > Using a smaller type complicates the DT for little to no benefit. I > think it's ok to use u32 here, which is a common standard for integer > values in DT. > > Though this discussion lead me to the question if we even need to have > this property in the DT at all. Isn't this a property that is fixed for > a specific silicon implementation of the DW core? In that case we could > just infer the number of ATUs from the DT compatible, so this should > probably just be added to struct pcie_port and properly initialized by > the SoC glue drivers. [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A implements 6 ATUs. > Regards, > Lucas >
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939: > Hi Lucas, > > Please see my comments inline. > > Thanks, > Minghuan > > On 2014?11?13? 00:32, Lucas Stach wrote: > > Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > >> Hi Minghuan, [...] > > Using a smaller type complicates the DT for little to no benefit. I > > think it's ok to use u32 here, which is a common standard for integer > > values in DT. > > > > Though this discussion lead me to the question if we even need to have > > this property in the DT at all. Isn't this a property that is fixed for > > a specific silicon implementation of the DW core? In that case we could > > just infer the number of ATUs from the DT compatible, so this should > > probably just be added to struct pcie_port and properly initialized by > > the SoC glue drivers. > [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why > pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A > implements 6 ATUs. > Right so we don't need an additional property in the DT at all. The number of ATUs is fixed for a specific core compatible and can be passed in by the respective exynos, imx and ls1021 glue drivers. You may ask the Keystone and Spear maintainers to get the correct number of ATUs for those implementations. Regards, Lucas
Hi Lucas, On 2014?11?13? 18:20, Lucas Stach wrote: > Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939: >> Hi Lucas, >> >> Please see my comments inline. >> >> Thanks, >> Minghuan >> >> On 2014?11?13? 00:32, Lucas Stach wrote: >>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: >>>> Hi Minghuan, > [...] > >>> Using a smaller type complicates the DT for little to no benefit. I >>> think it's ok to use u32 here, which is a common standard for integer >>> values in DT. >>> >>> Though this discussion lead me to the question if we even need to have >>> this property in the DT at all. Isn't this a property that is fixed for >>> a specific silicon implementation of the DW core? In that case we could >>> just infer the number of ATUs from the DT compatible, so this should >>> probably just be added to struct pcie_port and properly initialized by >>> the SoC glue drivers. >> [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why >> pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A >> implements 6 ATUs. >> > Right so we don't need an additional property in the DT at all. The > number of ATUs is fixed for a specific core compatible and can be passed > in by the respective exynos, imx and ls1021 glue drivers. > > You may ask the Keystone and Spear maintainers to get the correct number > of ATUs for those implementations. > > Regards, > Lucas [Minghuan] Yes. This a way that specific core driver passes the ATU number to pci-designware. But I perfer to adding dts node for the following reasons: 1. ATU number is hardware attribute, so it can be added to DTS. 2. That pci-designware common code parses the 'num-atus' can avoid every specific controller driver to define and pass num-atus, so can reduce code size and simplify the specific controller driver implementation. Thanks, Minghuan
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939: > Hi Lucas, > > On 2014?11?13? 18:20, Lucas Stach wrote: > > Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939: > >> Hi Lucas, > >> > >> Please see my comments inline. > >> > >> Thanks, > >> Minghuan > >> > >> On 2014?11?13? 00:32, Lucas Stach wrote: > >>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > >>>> Hi Minghuan, > > [...] > > > >>> Using a smaller type complicates the DT for little to no benefit. I > >>> think it's ok to use u32 here, which is a common standard for integer > >>> values in DT. > >>> > >>> Though this discussion lead me to the question if we even need to have > >>> this property in the DT at all. Isn't this a property that is fixed for > >>> a specific silicon implementation of the DW core? In that case we could > >>> just infer the number of ATUs from the DT compatible, so this should > >>> probably just be added to struct pcie_port and properly initialized by > >>> the SoC glue drivers. > >> [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why > >> pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A > >> implements 6 ATUs. > >> > > Right so we don't need an additional property in the DT at all. The > > number of ATUs is fixed for a specific core compatible and can be passed > > in by the respective exynos, imx and ls1021 glue drivers. > > > > You may ask the Keystone and Spear maintainers to get the correct number > > of ATUs for those implementations. > > > > Regards, > > Lucas > [Minghuan] Yes. This a way that specific core driver passes the ATU > number to > pci-designware. But I perfer to adding dts node for the following reasons: > 1. ATU number is hardware attribute, so it can be added to DTS. But it is a duplication of information that can be inferred from the DT compatible alone, which is usually frowned upon. Also in contrast to the num-lanes property I don't see a use-case to reduce the number of used ATUs in a specific system, so num-atus is basically fixed for a specific implementation. > 2. That pci-designware common code parses the 'num-atus' can avoid every > specific controller driver to define and pass num-atus, so can reduce > code size and simplify the specific controller driver implementation. > I don't think the code reduction matters here and the simplification is minimal. Regards, Lucas
Hi Lucas and all, On 2014?11?13? 19:09, Lucas Stach wrote: > Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939: >> Hi Lucas, >> >> On 2014?11?13? 18:20, Lucas Stach wrote: >>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939: >>>> Hi Lucas, >>>> >>>> Please see my comments inline. >>>> >>>> Thanks, >>>> Minghuan >>>> >>>> On 2014?11?13? 00:32, Lucas Stach wrote: >>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: >>>>>> Hi Minghuan, >>> [...] >>> >>>>> Using a smaller type complicates the DT for little to no benefit. I >>>>> think it's ok to use u32 here, which is a common standard for integer >>>>> values in DT. >>>>> >>>>> Though this discussion lead me to the question if we even need to have >>>>> this property in the DT at all. Isn't this a property that is fixed for >>>>> a specific silicon implementation of the DW core? In that case we could >>>>> just infer the number of ATUs from the DT compatible, so this should >>>>> probably just be added to struct pcie_port and properly initialized by >>>>> the SoC glue drivers. >>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why >>>> pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A >>>> implements 6 ATUs. >>>> >>> Right so we don't need an additional property in the DT at all. The >>> number of ATUs is fixed for a specific core compatible and can be passed >>> in by the respective exynos, imx and ls1021 glue drivers. >>> >>> You may ask the Keystone and Spear maintainers to get the correct number >>> of ATUs for those implementations. >>> >>> Regards, >>> Lucas >> [Minghuan] Yes. This a way that specific core driver passes the ATU >> number to >> pci-designware. But I perfer to adding dts node for the following reasons: >> 1. ATU number is hardware attribute, so it can be added to DTS. > But it is a duplication of information that can be inferred from the DT > compatible alone, which is usually frowned upon. > > Also in contrast to the num-lanes property I don't see a use-case to > reduce the number of used ATUs in a specific system, so num-atus is > basically fixed for a specific implementation. [Minghuan] 4ATUs provides better support for multiple-function and SR-IOV PCIe devices. Can anyone tell me if there is the company will increase ATUs number? If yes, we should avoid the following code: if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) atu_num = 2; if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) atu_num = 4; >> 2. That pci-designware common code parses the 'num-atus' can avoid every >> specific controller driver to define and pass num-atus, so can reduce >> code size and simplify the specific controller driver implementation. >> > I don't think the code reduction matters here and the simplification is > minimal. > > Regards, > Lucas >
On 2014?11?13? 00:23, Srikanth Thokala wrote: > Hi Minghuan, > > On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 > <B31939@freescale.com> wrote: > [...] > return ret; > @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > u32 membase; > u32 memlimit; > > + /* set ATUs setting for MEM and IO */ > + dw_pcie_prog_viewport_mem_outbound(pp); > + dw_pcie_prog_viewport_io_outbound(pp); > + >>>>> I see from the code, these settings are required for the calls other >>>>> than >>>>> dw_pcie_(rd/wr)_other_conf, is it correct? If it is so, I feel this >>>>> change >>>>> is >>>>> independent of this patch and should go as a separte patch? >>>> [Minghuan] dw_pcie(rd/wr)_other_confg only calls the >>>> dw_pcie_prog_viewport_mem/io_outbound when >>>> we only use 2 ATUs. >>>> The patch is to support 4ATUs. If no the calls, ATU2(MEM) and ATU3 will >>>> not be initialized, and PCIe device driver will be broken. >>> When you have only 2 ATUs (CFG0=MEM & CFG1=IO), you are calling >>> mem/io_outbound() twice with the same writes which is not the case in the >>> original driver. So, I mentioned it should go as a separate patch. >> [Minghuan] Sorry, I do not understand what you mean. >> How to separate patch? >> A patch is to add the following code based on original code >> >> + /* set ATUs setting for MEM and IO */ >> + dw_pcie_prog_viewport_mem_outbound(pp); >> + dw_pcie_prog_viewport_io_outbound(pp); >> + >> >> But why add this patch? 2 ATUs does not need them. >> >> Only 4 ATUs support needs them. > Then you may have to add a condition here, num_atus >= 4? > >> And them are also ok for 2 ATUs. > You are just re-writing them anyway, so I don't see a place for them here. > > So, this fragment should just work, > > +++ > if (num_atus >=4 ) { > dw_pcie_prog_viewport_mem_outbound(pp); > dw_pcie_prog_viewport_io_outbound(pp); > } > +++ > > Is that correct? Am I still missing? [Minghuan] For 2 ATUs, ATU0 is for MEM as default, ATU1 for IO. When to access CFG0/CFG1, ATU will temporarily to be changed to CFG0/CFG1, then will be changed back to MEM/IO. So the mem/io initialization I added will not bring any harm for 2 ATUs. Why do we need the judgement 'num_atus >=4'. >> For 2 ATUs, mem/io will be initialized every time read/write PCI device >> configuration. > Just out of curiosity, is it really required to initialize mem/io everytime > there is a config read/write? Would it not work when initialized just once like > for the case of 4 ATUs? [Minghuan] For 2 ATUs, CFG0 and MEM share ATU0. So when access PCIe device configuration ATU0 will be changed to CFG0 setting, and then be changed to MEM setting for MEM support. > - Srikanth > >> >>
Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939: > Hi Lucas and all, > > On 2014?11?13? 19:09, Lucas Stach wrote: > > Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939: > >> Hi Lucas, > >> > >> On 2014?11?13? 18:20, Lucas Stach wrote: > >>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939: > >>>> Hi Lucas, > >>>> > >>>> Please see my comments inline. > >>>> > >>>> Thanks, > >>>> Minghuan > >>>> > >>>> On 2014?11?13? 00:32, Lucas Stach wrote: > >>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > >>>>>> Hi Minghuan, > >>> [...] > >>> > >>>>> Using a smaller type complicates the DT for little to no benefit. I > >>>>> think it's ok to use u32 here, which is a common standard for integer > >>>>> values in DT. > >>>>> > >>>>> Though this discussion lead me to the question if we even need to have > >>>>> this property in the DT at all. Isn't this a property that is fixed for > >>>>> a specific silicon implementation of the DW core? In that case we could > >>>>> just infer the number of ATUs from the DT compatible, so this should > >>>>> probably just be added to struct pcie_port and properly initialized by > >>>>> the SoC glue drivers. > >>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this is why > >>>> pcie-designware only supports 2 ATU. iMX implements 4 ATUs and LS1021A > >>>> implements 6 ATUs. > >>>> > >>> Right so we don't need an additional property in the DT at all. The > >>> number of ATUs is fixed for a specific core compatible and can be passed > >>> in by the respective exynos, imx and ls1021 glue drivers. > >>> > >>> You may ask the Keystone and Spear maintainers to get the correct number > >>> of ATUs for those implementations. > >>> > >>> Regards, > >>> Lucas > >> [Minghuan] Yes. This a way that specific core driver passes the ATU > >> number to > >> pci-designware. But I perfer to adding dts node for the following reasons: > >> 1. ATU number is hardware attribute, so it can be added to DTS. > > But it is a duplication of information that can be inferred from the DT > > compatible alone, which is usually frowned upon. > > > > Also in contrast to the num-lanes property I don't see a use-case to > > reduce the number of used ATUs in a specific system, so num-atus is > > basically fixed for a specific implementation. > [Minghuan] 4ATUs provides better support for multiple-function and > SR-IOV PCIe devices. > Can anyone tell me if there is the company will increase ATUs number? > If yes, we should avoid the following code: > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) > atu_num = 2; > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) > atu_num = 4; > The number of ATUs is fixed for a specific implementation. If the number of ATUs changes in a newer implementation of a SoC then the silicon implementation of the DW PCIe core changed, so it should get a new compatible anyway. Regards, Lucas
> -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: Friday, November 14, 2014 6:02 PM > To: Lian Minghuan-B31939 > Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; > Bjorn Helgaas > Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment > > Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939: > > Hi Lucas and all, > > > > On 2014?11?13? 19:09, Lucas Stach wrote: > > > Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan- > B31939: > > >> Hi Lucas, > > >> > > >> On 2014?11?13? 18:20, Lucas Stach wrote: > > >>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan- > B31939: > > >>>> Hi Lucas, > > >>>> > > >>>> Please see my comments inline. > > >>>> > > >>>> Thanks, > > >>>> Minghuan > > >>>> > > >>>> On 2014?11?13? 00:32, Lucas Stach wrote: > > >>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > > >>>>>> Hi Minghuan, > > >>> [...] > > >>> > > >>>>> Using a smaller type complicates the DT for little to no > > >>>>> benefit. I think it's ok to use u32 here, which is a common > > >>>>> standard for integer values in DT. > > >>>>> > > >>>>> Though this discussion lead me to the question if we even need > > >>>>> to have this property in the DT at all. Isn't this a property > > >>>>> that is fixed for a specific silicon implementation of the DW > > >>>>> core? In that case we could just infer the number of ATUs from > > >>>>> the DT compatible, so this should probably just be added to > > >>>>> struct pcie_port and properly initialized by the SoC glue drivers. > > >>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this > > >>>> is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs > > >>>> and LS1021A implements 6 ATUs. > > >>>> > > >>> Right so we don't need an additional property in the DT at all. > > >>> The number of ATUs is fixed for a specific core compatible and can > > >>> be passed in by the respective exynos, imx and ls1021 glue drivers. > > >>> > > >>> You may ask the Keystone and Spear maintainers to get the correct > > >>> number of ATUs for those implementations. > >>> > > >>> Regards, > > >>> Lucas > > >> [Minghuan] Yes. This a way that specific core driver passes the ATU > > >> number to pci-designware. But I perfer to adding dts node for the > > >> following reasons: > > >> 1. ATU number is hardware attribute, so it can be added to DTS. > > > But it is a duplication of information that can be inferred from the > > > DT compatible alone, which is usually frowned upon. > > > > > > Also in contrast to the num-lanes property I don't see a use-case to > > > reduce the number of used ATUs in a specific system, so num-atus is > > > basically fixed for a specific implementation. > > [Minghuan] 4ATUs provides better support for multiple-function and > > SR-IOV PCIe devices. > > Can anyone tell me if there is the company will increase ATUs number? > > If yes, we should avoid the following code: > > > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) > > atu_num = 2; > > > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) > > atu_num = 4; > > > The number of ATUs is fixed for a specific implementation. If the number > of ATUs changes in a newer implementation of a SoC then the silicon > implementation of the DW PCIe core changed, so it should get a new > compatible anyway. > If there are multiple platforms to use the same SoC driver, there will be multiple cases for different platforms as Minghuan has pointed out. For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and there will be conflict when MEM and CFG0 are accessed simultaneously which has cause the SATA link issue. I think more ATUs will be added in the future design, then one driver maybe need to handle multiple platforms. DTS is designed for to describe the hardware property, it's a general way to put hardware related property into DTS. Thanks, Mingkai
Am Freitag, den 14.11.2014, 11:30 +0000 schrieb Mingkai.Hu@freescale.com: > > > > -----Original Message----- > > From: Lucas Stach [mailto:l.stach@pengutronix.de] > > Sent: Friday, November 14, 2014 6:02 PM > > To: Lian Minghuan-B31939 > > Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci@vger.kernel.org; > > linux-arm-kernel@lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; > > Bjorn Helgaas > > Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment > > > > Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939: > > > Hi Lucas and all, > > > > > > On 2014?11?13? 19:09, Lucas Stach wrote: > > > > Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan- > > B31939: > > > >> Hi Lucas, > > > >> > > > >> On 2014?11?13? 18:20, Lucas Stach wrote: > > > >>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan- > > B31939: > > > >>>> Hi Lucas, > > > >>>> > > > >>>> Please see my comments inline. > > > >>>> > > > >>>> Thanks, > > > >>>> Minghuan > > > >>>> > > > >>>> On 2014?11?13? 00:32, Lucas Stach wrote: > > > >>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > > > >>>>>> Hi Minghuan, > > > >>> [...] > > > >>> > > > >>>>> Using a smaller type complicates the DT for little to no > > > >>>>> benefit. I think it's ok to use u32 here, which is a common > > > >>>>> standard for integer values in DT. > > > >>>>> > > > >>>>> Though this discussion lead me to the question if we even need > > > >>>>> to have this property in the DT at all. Isn't this a property > > > >>>>> that is fixed for a specific silicon implementation of the DW > > > >>>>> core? In that case we could just infer the number of ATUs from > > > >>>>> the DT compatible, so this should probably just be added to > > > >>>>> struct pcie_port and properly initialized by the SoC glue drivers. > > > >>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this > > > >>>> is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs > > > >>>> and LS1021A implements 6 ATUs. > > > >>>> > > > >>> Right so we don't need an additional property in the DT at all. > > > >>> The number of ATUs is fixed for a specific core compatible and can > > > >>> be passed in by the respective exynos, imx and ls1021 glue drivers. > > > >>> > > > >>> You may ask the Keystone and Spear maintainers to get the correct > > > >>> number of ATUs for those implementations. > > >>> > > > >>> Regards, > > > >>> Lucas > > > >> [Minghuan] Yes. This a way that specific core driver passes the ATU > > > >> number to pci-designware. But I perfer to adding dts node for the > > > >> following reasons: > > > >> 1. ATU number is hardware attribute, so it can be added to DTS. > > > > But it is a duplication of information that can be inferred from the > > > > DT compatible alone, which is usually frowned upon. > > > > > > > > Also in contrast to the num-lanes property I don't see a use-case to > > > > reduce the number of used ATUs in a specific system, so num-atus is > > > > basically fixed for a specific implementation. > > > [Minghuan] 4ATUs provides better support for multiple-function and > > > SR-IOV PCIe devices. > > > Can anyone tell me if there is the company will increase ATUs number? > > > If yes, we should avoid the following code: > > > > > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) > > > atu_num = 2; > > > > > > if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) > > > atu_num = 4; > > > > > The number of ATUs is fixed for a specific implementation. If the number > > of ATUs changes in a newer implementation of a SoC then the silicon > > implementation of the DW PCIe core changed, so it should get a new > > compatible anyway. > > > > If there are multiple platforms to use the same SoC driver, there will be > multiple cases for different platforms as Minghuan has pointed out. > > For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and > there will be conflict when MEM and CFG0 are accessed simultaneously which > has cause the SATA link issue. I think more ATUs will be added in the > future design, then one driver maybe need to handle multiple platforms. > > DTS is designed for to describe the hardware property, it's a general way > to put hardware related property into DTS. > If another platform uses the same driver you should still have a specific compatible for that platform. Exactly to differentiate those silicon implementation differences. If you add 2 more ATUs to the layerscape pcie in a future design it is not fsl,ls1021a-pcie anymore, but for example fsl,ls1100-pcie. Also you don't need the code construct as seen above. The number of ATU can be added to implementation specific data. Please look at the Tegra PCI driver and especially struct tegra_pcie_soc_data on how to properly abstract properties that are defined by a specific silicon implementation of a core. Regards, Lucas
Hi Lucas, On 2014?11?14? 19:42, Lucas Stach wrote: > Am Freitag, den 14.11.2014, 11:30 +0000 schrieb > Mingkai.Hu@freescale.com: >> >>> -----Original Message----- >>> From: Lucas Stach [mailto:l.stach@pengutronix.de] >>> Sent: Friday, November 14, 2014 6:02 PM >>> To: Lian Minghuan-B31939 >>> Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci@vger.kernel.org; >>> linux-arm-kernel@lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; >>> Bjorn Helgaas >>> Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment >>> >>> Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939: >>>> Hi Lucas and all, >>>> >>>> On 2014?11?13? 19:09, Lucas Stach wrote: >>>>> Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan- >>> B31939: >>>>>> Hi Lucas, >>>>>> >>>>>> On 2014?11?13? 18:20, Lucas Stach wrote: >>>>>>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan- >>> B31939: >>>>>>>> Hi Lucas, >>>>>>>> >>>>>>>> Please see my comments inline. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Minghuan >>>>>>>> >>>>>>>> On 2014?11?13? 00:32, Lucas Stach wrote: >>>>>>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: >>>>>>>>>> Hi Minghuan, >>>>>>> [...] >>>>>>> >>>>>>>>> Using a smaller type complicates the DT for little to no >>>>>>>>> benefit. I think it's ok to use u32 here, which is a common >>>>>>>>> standard for integer values in DT. >>>>>>>>> >>>>>>>>> Though this discussion lead me to the question if we even need >>>>>>>>> to have this property in the DT at all. Isn't this a property >>>>>>>>> that is fixed for a specific silicon implementation of the DW >>>>>>>>> core? In that case we could just infer the number of ATUs from >>>>>>>>> the DT compatible, so this should probably just be added to >>>>>>>>> struct pcie_port and properly initialized by the SoC glue drivers. >>>>>>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this >>>>>>>> is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs >>>>>>>> and LS1021A implements 6 ATUs. >>>>>>>> >>>>>>> Right so we don't need an additional property in the DT at all. >>>>>>> The number of ATUs is fixed for a specific core compatible and can >>>>>>> be passed in by the respective exynos, imx and ls1021 glue drivers. >>>>>>> >>>>>>> You may ask the Keystone and Spear maintainers to get the correct >>>>>>> number of ATUs for those implementations. >> > >>> >>>>>>> Regards, >>>>>>> Lucas >>>>>> [Minghuan] Yes. This a way that specific core driver passes the ATU >>>>>> number to pci-designware. But I perfer to adding dts node for the >>>>>> following reasons: >>>>>> 1. ATU number is hardware attribute, so it can be added to DTS. >>>>> But it is a duplication of information that can be inferred from the >>>>> DT compatible alone, which is usually frowned upon. >>>>> >>>>> Also in contrast to the num-lanes property I don't see a use-case to >>>>> reduce the number of used ATUs in a specific system, so num-atus is >>>>> basically fixed for a specific implementation. >>>> [Minghuan] 4ATUs provides better support for multiple-function and >>>> SR-IOV PCIe devices. >>>> Can anyone tell me if there is the company will increase ATUs number? >>>> If yes, we should avoid the following code: >>>> >>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) >>>> atu_num = 2; >>>> >>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) >>>> atu_num = 4; >>>> >>> The number of ATUs is fixed for a specific implementation. If the number >>> of ATUs changes in a newer implementation of a SoC then the silicon >>> implementation of the DW PCIe core changed, so it should get a new >>> compatible anyway. >>> >> If there are multiple platforms to use the same SoC driver, there will be >> multiple cases for different platforms as Minghuan has pointed out. >> >> For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and >> there will be conflict when MEM and CFG0 are accessed simultaneously which >> has cause the SATA link issue. I think more ATUs will be added in the >> future design, then one driver maybe need to handle multiple platforms. >> >> DTS is designed for to describe the hardware property, it's a general way >> to put hardware related property into DTS. >> > If another platform uses the same driver you should still have a > specific compatible for that platform. Exactly to differentiate those > silicon implementation differences. > > If you add 2 more ATUs to the layerscape pcie in a future design it is > not fsl,ls1021a-pcie anymore, but for example fsl,ls1100-pcie. Also you > don't need the code construct as seen above. The number of ATU can be > added to implementation specific data. > Please look at the Tegra PCI driver and especially struct > tegra_pcie_soc_data on how to properly abstract properties that are > defined by a specific silicon implementation of a core. [Minghuan] Yes, we can added specific data for different implementation. But we need to add hard coded information and assignment statement to the layerscape PCIe driver even to every PCIe driver based on pcie-designware.c. Why not use dts? Nothing needs to be added to specific implementation driver code. The Device Tree is a data structure for describing hardware. Rather than hard coding every detail of a device into an operating system, many aspect of the hardware can be described in a data structure that is passed to the operating system at boot time. Thanks, Minghuan > Regards, > Lucas >
Rob, Mark, Kumar, Ian, could I please get your opinion on the following matter? Thanks. Am Montag, den 17.11.2014, 10:58 +0800 schrieb Lian Minghuan-B31939: > Hi Lucas, > > On 2014?11?14? 19:42, Lucas Stach wrote: > > Am Freitag, den 14.11.2014, 11:30 +0000 schrieb > > Mingkai.Hu@freescale.com: > >> > >>> -----Original Message----- > >>> From: Lucas Stach [mailto:l.stach@pengutronix.de] > >>> Sent: Friday, November 14, 2014 6:02 PM > >>> To: Lian Minghuan-B31939 > >>> Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci@vger.kernel.org; > >>> linux-arm-kernel@lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; > >>> Bjorn Helgaas > >>> Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment > >>> > >>> Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939: > >>>> Hi Lucas and all, > >>>> > >>>> On 2014?11?13? 19:09, Lucas Stach wrote: > >>>>> Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan- > >>> B31939: > >>>>>> Hi Lucas, > >>>>>> > >>>>>> On 2014?11?13? 18:20, Lucas Stach wrote: > >>>>>>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan- > >>> B31939: > >>>>>>>> Hi Lucas, > >>>>>>>> > >>>>>>>> Please see my comments inline. > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Minghuan > >>>>>>>> > >>>>>>>> On 2014?11?13? 00:32, Lucas Stach wrote: > >>>>>>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala: > >>>>>>>>>> Hi Minghuan, > >>>>>>> [...] > >>>>>>> > >>>>>>>>> Using a smaller type complicates the DT for little to no > >>>>>>>>> benefit. I think it's ok to use u32 here, which is a common > >>>>>>>>> standard for integer values in DT. > >>>>>>>>> > >>>>>>>>> Though this discussion lead me to the question if we even need > >>>>>>>>> to have this property in the DT at all. Isn't this a property > >>>>>>>>> that is fixed for a specific silicon implementation of the DW > >>>>>>>>> core? In that case we could just infer the number of ATUs from > >>>>>>>>> the DT compatible, so this should probably just be added to > >>>>>>>>> struct pcie_port and properly initialized by the SoC glue drivers. > >>>>>>>> [Minghuan] As far as I know, exynos implements only 2 ATUs, this > >>>>>>>> is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs > >>>>>>>> and LS1021A implements 6 ATUs. > >>>>>>>> > >>>>>>> Right so we don't need an additional property in the DT at all. > >>>>>>> The number of ATUs is fixed for a specific core compatible and can > >>>>>>> be passed in by the respective exynos, imx and ls1021 glue drivers. > >>>>>>> > >>>>>>> You may ask the Keystone and Spear maintainers to get the correct > >>>>>>> number of ATUs for those implementations. > >> > >>> > >>>>>>> Regards, > >>>>>>> Lucas > >>>>>> [Minghuan] Yes. This a way that specific core driver passes the ATU > >>>>>> number to pci-designware. But I perfer to adding dts node for the > >>>>>> following reasons: > >>>>>> 1. ATU number is hardware attribute, so it can be added to DTS. > >>>>> But it is a duplication of information that can be inferred from the > >>>>> DT compatible alone, which is usually frowned upon. > >>>>> > >>>>> Also in contrast to the num-lanes property I don't see a use-case to > >>>>> reduce the number of used ATUs in a specific system, so num-atus is > >>>>> basically fixed for a specific implementation. > >>>> [Minghuan] 4ATUs provides better support for multiple-function and > >>>> SR-IOV PCIe devices. > >>>> Can anyone tell me if there is the company will increase ATUs number? > >>>> If yes, we should avoid the following code: > >>>> > >>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie")) > >>>> atu_num = 2; > >>>> > >>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie")) > >>>> atu_num = 4; > >>>> > >>> The number of ATUs is fixed for a specific implementation. If the number > >>> of ATUs changes in a newer implementation of a SoC then the silicon > >>> implementation of the DW PCIe core changed, so it should get a new > >>> compatible anyway. > >>> > >> If there are multiple platforms to use the same SoC driver, there will be > >> multiple cases for different platforms as Minghuan has pointed out. > >> > >> For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and > >> there will be conflict when MEM and CFG0 are accessed simultaneously which > >> has cause the SATA link issue. I think more ATUs will be added in the > >> future design, then one driver maybe need to handle multiple platforms. > >> > >> DTS is designed for to describe the hardware property, it's a general way > >> to put hardware related property into DTS. > >> > > If another platform uses the same driver you should still have a > > specific compatible for that platform. Exactly to differentiate those > > silicon implementation differences. > > > > If you add 2 more ATUs to the layerscape pcie in a future design it is > > not fsl,ls1021a-pcie anymore, but for example fsl,ls1100-pcie. Also you > > don't need the code construct as seen above. The number of ATU can be > > added to implementation specific data. > > Please look at the Tegra PCI driver and especially struct > > tegra_pcie_soc_data on how to properly abstract properties that are > > defined by a specific silicon implementation of a core. > [Minghuan] Yes, we can added specific data for different implementation. > But we need to add hard coded information and assignment statement to > the layerscape PCIe driver even to every PCIe driver based on > pcie-designware.c. > > Why not use dts? Nothing needs to be added to specific implementation > driver code. > This is an implementation detail of the Linux driver. We generally don't decide about the DT layout based on implementation details. > The Device Tree is a data structure for describing hardware. Rather than > hard coding every detail of a device into an operating system, many > aspect of the hardware can be described in a data structure that is > passed to the operating system at boot time. > Because in my view this is a duplication of information. I would argue that we should not put any properties into the DT that can be easily inferred from the device compatible alone. I can already see problems with DT backwards compatibility for this property. You can not make the property mandatory as it this would break old DTs. You now assume that if the property isn't present it means that 2 ATUs are implemented. While it isn't strictly breaking anything having the property in the DT means that only systems with a new DTB will set the right number of ATUs (4) for i.MX6. This could be easily avoided if we just infer the property from the compatible. But I'm no authority on DT decisions, let's get the proper people on CC so we can get to an agreement here. Regards, Lucas
diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt index 9f4faa8..64d0533 100644 --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt @@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to specify this property, to keep backwards compatibility a range of 0x00-0xff is assumed if not present) +- num-atus: number of ATUs. The default value is 2 if not present. diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index dfed00a..46a609d 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -48,6 +48,8 @@ #define PCIE_ATU_VIEWPORT 0x900 #define PCIE_ATU_REGION_INBOUND (0x1 << 31) #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) +#define PCIE_ATU_REGION_INDEX3 (0x3 << 0) +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) #define PCIE_ATU_CR1 0x904 @@ -346,7 +348,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) struct of_pci_range range; struct of_pci_range_parser parser; struct resource *cfg_res; - u32 val, na, ns; + u32 num_atus = 2, val, na, ns; const __be32 *addrp; int i, index, ret; @@ -486,6 +488,19 @@ int __init dw_pcie_host_init(struct pcie_port *pp) } } + of_property_read_u32(np, "num-atus", &num_atus); + if (num_atus >= 4) { + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX2; + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX3; + } else { + pp->atu_idx[ATU_TYPE_CFG0] = PCIE_ATU_REGION_INDEX0; + pp->atu_idx[ATU_TYPE_MEM] = PCIE_ATU_REGION_INDEX0; + pp->atu_idx[ATU_TYPE_CFG1] = PCIE_ATU_REGION_INDEX1; + pp->atu_idx[ATU_TYPE_IO] = PCIE_ATU_REGION_INDEX1; + } + if (pp->ops->host_init) pp->ops->host_init(pp); @@ -511,8 +526,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp) static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) { - /* Program viewport 0 : OUTBOUND : CFG0 */ - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, + /* Program viewport : OUTBOUND : CFG0 */ + dw_pcie_writel_rc(pp, + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG0], PCIE_ATU_VIEWPORT); dw_pcie_writel_rc(pp, pp->cfg0_mod_base, PCIE_ATU_LOWER_BASE); dw_pcie_writel_rc(pp, (pp->cfg0_mod_base >> 32), PCIE_ATU_UPPER_BASE); @@ -526,8 +542,9 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) { - /* Program viewport 1 : OUTBOUND : CFG1 */ - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, + /* Program viewport : OUTBOUND : CFG1 */ + dw_pcie_writel_rc(pp, + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_CFG1], PCIE_ATU_VIEWPORT); dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); dw_pcie_writel_rc(pp, pp->cfg1_mod_base, PCIE_ATU_LOWER_BASE); @@ -541,8 +558,9 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) { - /* Program viewport 0 : OUTBOUND : MEM */ - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0, + /* Program viewport : OUTBOUND : MEM */ + dw_pcie_writel_rc(pp, + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_MEM], PCIE_ATU_VIEWPORT); dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); dw_pcie_writel_rc(pp, pp->mem_mod_base, PCIE_ATU_LOWER_BASE); @@ -557,8 +575,9 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) { - /* Program viewport 1 : OUTBOUND : IO */ - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1, + /* Program viewport : OUTBOUND : IO */ + dw_pcie_writel_rc(pp, + PCIE_ATU_REGION_OUTBOUND | pp->atu_idx[ATU_TYPE_IO], PCIE_ATU_VIEWPORT); dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); dw_pcie_writel_rc(pp, pp->io_mod_base, PCIE_ATU_LOWER_BASE); @@ -585,12 +604,14 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, dw_pcie_prog_viewport_cfg0(pp, busdev); ret = dw_pcie_cfg_read(pp->va_cfg0_base + address, where, size, val); - dw_pcie_prog_viewport_mem_outbound(pp); + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) + dw_pcie_prog_viewport_mem_outbound(pp); } else { dw_pcie_prog_viewport_cfg1(pp, busdev); ret = dw_pcie_cfg_read(pp->va_cfg1_base + address, where, size, val); - dw_pcie_prog_viewport_io_outbound(pp); + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) + dw_pcie_prog_viewport_io_outbound(pp); } return ret; @@ -610,12 +631,14 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, dw_pcie_prog_viewport_cfg0(pp, busdev); ret = dw_pcie_cfg_write(pp->va_cfg0_base + address, where, size, val); - dw_pcie_prog_viewport_mem_outbound(pp); + if (pp->atu_idx[ATU_TYPE_MEM] == pp->atu_idx[ATU_TYPE_CFG0]) + dw_pcie_prog_viewport_mem_outbound(pp); } else { dw_pcie_prog_viewport_cfg1(pp, busdev); ret = dw_pcie_cfg_write(pp->va_cfg1_base + address, where, size, val); - dw_pcie_prog_viewport_io_outbound(pp); + if (pp->atu_idx[ATU_TYPE_IO] == pp->atu_idx[ATU_TYPE_CFG1]) + dw_pcie_prog_viewport_io_outbound(pp); } return ret; @@ -770,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) u32 membase; u32 memlimit; + /* set ATUs setting for MEM and IO */ + dw_pcie_prog_viewport_mem_outbound(pp); + dw_pcie_prog_viewport_io_outbound(pp); + /* set the number of lanes */ dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL, &val); val &= ~PORT_LINK_MODE_MASK; diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index c625675..37604f9 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -22,6 +22,14 @@ #define MAX_MSI_IRQS 32 #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) +enum ATU_TYPE { + ATU_TYPE_CFG0, + ATU_TYPE_CFG1, + ATU_TYPE_MEM, + ATU_TYPE_IO, + ATU_TYPE_MAX +}; + struct pcie_port { struct device *dev; u8 root_bus_nr; @@ -53,6 +61,7 @@ struct pcie_port { struct irq_domain *irq_domain; unsigned long msi_data; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); + u8 atu_idx[ATU_TYPE_MAX]; }; struct pcie_host_ops {
Currently, pcie-designware.c only supports two ATUs, ATU0 is used for CFG0 and MEM, ATU1 is used for CFG1 and IO. There is a conflict when MEM and CFG0 are accessed simultaneously. The patch adds 'num-atus' property to PCIe dts node to describe the number of PCIe controller's ATUs. If num_atus is bigger than or equal to 4, we will change ATUs assignment: ATU0 for CFG0, ATU1 for CFG1, ATU2 for MEM, ATU3 for IO. Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> --- change log: v1-v2: 1. add the default value to property num-atus description 2. Use atu_idx[] instead of single values 3. initialize num_atus to 2 .../devicetree/bindings/pci/designware-pcie.txt | 1 + drivers/pci/host/pcie-designware.c | 53 ++++++++++++++++------ drivers/pci/host/pcie-designware.h | 9 ++++ 3 files changed, 50 insertions(+), 13 deletions(-)