* [PATCH v2] PCI: designware: Add support 4 ATUs assignment @ 2014-11-11 5:07 Minghuan Lian 2014-11-12 6:22 ` Srikanth Thokala 0 siblings, 1 reply; 18+ messages in thread From: Minghuan Lian @ 2014-11-11 5:07 UTC (permalink / raw) To: linux-pci Cc: linux-arm-kernel, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach, Minghuan Lian 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; 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 { -- 1.9.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-11 5:07 [PATCH v2] PCI: designware: Add support 4 ATUs assignment Minghuan Lian @ 2014-11-12 6:22 ` Srikanth Thokala 2014-11-12 7:14 ` Lian Minghuan-B31939 0 siblings, 1 reply; 18+ messages in thread From: Srikanth Thokala @ 2014-11-12 6:22 UTC (permalink / raw) To: Minghuan Lian Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 6:22 ` Srikanth Thokala @ 2014-11-12 7:14 ` Lian Minghuan-B31939 2014-11-12 9:01 ` Srikanth Thokala 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-12 7:14 UTC (permalink / raw) To: Srikanth Thokala, Minghuan Lian Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 7:14 ` Lian Minghuan-B31939 @ 2014-11-12 9:01 ` Srikanth Thokala 2014-11-12 10:09 ` Lian Minghuan-B31939 0 siblings, 1 reply; 18+ messages in thread From: Srikanth Thokala @ 2014-11-12 9:01 UTC (permalink / raw) To: Minghuan Lian Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 9:01 ` Srikanth Thokala @ 2014-11-12 10:09 ` Lian Minghuan-B31939 2014-11-12 16:23 ` Srikanth Thokala 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-12 10:09 UTC (permalink / raw) To: Srikanth Thokala, Minghuan Lian Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 10:09 ` Lian Minghuan-B31939 @ 2014-11-12 16:23 ` Srikanth Thokala 2014-11-12 16:32 ` Lucas Stach 2014-11-14 9:36 ` Lian Minghuan-B31939 0 siblings, 2 replies; 18+ messages in thread From: Srikanth Thokala @ 2014-11-12 16:23 UTC (permalink / raw) To: Lian Minghuan-B31939 Cc: Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 >>> >>> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 16:23 ` Srikanth Thokala @ 2014-11-12 16:32 ` Lucas Stach 2014-11-13 10:02 ` Lian Minghuan-B31939 2014-11-14 9:36 ` Lian Minghuan-B31939 1 sibling, 1 reply; 18+ messages in thread From: Lucas Stach @ 2014-11-12 16:32 UTC (permalink / raw) To: Srikanth Thokala Cc: Lian Minghuan-B31939, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 16:32 ` Lucas Stach @ 2014-11-13 10:02 ` Lian Minghuan-B31939 2014-11-13 10:20 ` Lucas Stach 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-13 10:02 UTC (permalink / raw) To: Lucas Stach, Srikanth Thokala Cc: Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-13 10:02 ` Lian Minghuan-B31939 @ 2014-11-13 10:20 ` Lucas Stach 2014-11-13 10:52 ` Lian Minghuan-B31939 0 siblings, 1 reply; 18+ messages in thread From: Lucas Stach @ 2014-11-13 10:20 UTC (permalink / raw) To: Lian Minghuan-B31939 Cc: Srikanth Thokala, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-13 10:20 ` Lucas Stach @ 2014-11-13 10:52 ` Lian Minghuan-B31939 2014-11-13 11:09 ` Lucas Stach 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-13 10:52 UTC (permalink / raw) To: Lucas Stach Cc: Srikanth Thokala, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-13 10:52 ` Lian Minghuan-B31939 @ 2014-11-13 11:09 ` Lucas Stach 2014-11-14 8:47 ` Lian Minghuan-B31939 0 siblings, 1 reply; 18+ messages in thread From: Lucas Stach @ 2014-11-13 11:09 UTC (permalink / raw) To: Lian Minghuan-B31939 Cc: Srikanth Thokala, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-13 11:09 ` Lucas Stach @ 2014-11-14 8:47 ` Lian Minghuan-B31939 2014-11-14 10:02 ` Lucas Stach 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-14 8:47 UTC (permalink / raw) To: Lucas Stach Cc: Srikanth Thokala, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-14 8:47 ` Lian Minghuan-B31939 @ 2014-11-14 10:02 ` Lucas Stach 2014-11-14 11:30 ` Mingkai.Hu 0 siblings, 1 reply; 18+ messages in thread From: Lucas Stach @ 2014-11-14 10:02 UTC (permalink / raw) To: Lian Minghuan-B31939 Cc: Srikanth Thokala, Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-14 10:02 ` Lucas Stach @ 2014-11-14 11:30 ` Mingkai.Hu 2014-11-14 11:42 ` Lucas Stach 0 siblings, 1 reply; 18+ messages in thread From: Mingkai.Hu @ 2014-11-14 11:30 UTC (permalink / raw) To: Lucas Stach, Minghuan.Lian@freescale.com Cc: Srikanth Thokala, Minghuan.Lian@freescale.com, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Roy Zang, Bjorn Helgaas DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTHVjYXMgU3RhY2ggW21h aWx0bzpsLnN0YWNoQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiBGcmlkYXksIE5vdmVtYmVyIDE0 LCAyMDE0IDY6MDIgUE0NCj4gVG86IExpYW4gTWluZ2h1YW4tQjMxOTM5DQo+IENjOiBTcmlrYW50 aCBUaG9rYWxhOyBMaWFuIE1pbmdodWFuLUIzMTkzOTsgbGludXgtcGNpQHZnZXIua2VybmVsLm9y ZzsNCj4gbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBaYW5nIFJveS1SNjE5 MTE7IEh1IE1pbmdrYWktQjIxMjg0Ow0KPiBCam9ybiBIZWxnYWFzDQo+IFN1YmplY3Q6IFJlOiBb UEFUQ0ggdjJdIFBDSTogZGVzaWdud2FyZTogQWRkIHN1cHBvcnQgNCBBVFVzIGFzc2lnbm1lbnQN Cj4gDQo+IEFtIEZyZWl0YWcsIGRlbiAxNC4xMS4yMDE0LCAxNjo0NyArMDgwMCBzY2hyaWViIExp YW4gTWluZ2h1YW4tQjMxOTM5Og0KPiA+IEhpIEx1Y2FzIGFuZCBhbGwsDQo+ID4NCj4gPiBPbiAy MDE05bm0MTHmnIgxM+aXpSAxOTowOSwgTHVjYXMgU3RhY2ggd3JvdGU6DQo+ID4gPiBBbSBEb25u ZXJzdGFnLCBkZW4gMTMuMTEuMjAxNCwgMTg6NTIgKzA4MDAgc2NocmllYiBMaWFuIE1pbmdodWFu LQ0KPiBCMzE5Mzk6DQo+ID4gPj4gSGkgTHVjYXMsDQo+ID4gPj4NCj4gPiA+PiBPbiAyMDE05bm0 MTHmnIgxM+aXpSAxODoyMCwgTHVjYXMgU3RhY2ggd3JvdGU6DQo+ID4gPj4+IEFtIERvbm5lcnN0 YWcsIGRlbiAxMy4xMS4yMDE0LCAxODowMiArMDgwMCBzY2hyaWViIExpYW4gTWluZ2h1YW4tDQo+ IEIzMTkzOToNCj4gPiA+Pj4+IEhpIEx1Y2FzLA0KPiA+ID4+Pj4NCj4gPiA+Pj4+IFBsZWFzZSBz ZWUgbXkgY29tbWVudHMgaW5saW5lLg0KPiA+ID4+Pj4NCj4gPiA+Pj4+IFRoYW5rcywNCj4gPiA+ Pj4+IE1pbmdodWFuDQo+ID4gPj4+Pg0KPiA+ID4+Pj4gT24gMjAxNOW5tDEx5pyIMTPml6UgMDA6 MzIsIEx1Y2FzIFN0YWNoIHdyb3RlOg0KPiA+ID4+Pj4+IEFtIE1pdHR3b2NoLCBkZW4gMTIuMTEu MjAxNCwgMjE6NTMgKzA1MzAgc2NocmllYiBTcmlrYW50aCBUaG9rYWxhOg0KPiA+ID4+Pj4+PiBI aSBNaW5naHVhbiwNCj4gPiA+Pj4gWy4uLl0NCj4gPiA+Pj4NCj4gPiA+Pj4+PiBVc2luZyBhIHNt YWxsZXIgdHlwZSBjb21wbGljYXRlcyB0aGUgRFQgZm9yIGxpdHRsZSB0byBubw0KPiA+ID4+Pj4+ IGJlbmVmaXQuIEkgdGhpbmsgaXQncyBvayB0byB1c2UgdTMyIGhlcmUsIHdoaWNoIGlzIGEgY29t bW9uDQo+ID4gPj4+Pj4gc3RhbmRhcmQgZm9yIGludGVnZXIgdmFsdWVzIGluIERULg0KPiA+ID4+ Pj4+DQo+ID4gPj4+Pj4gVGhvdWdoIHRoaXMgZGlzY3Vzc2lvbiBsZWFkIG1lIHRvIHRoZSBxdWVz dGlvbiBpZiB3ZSBldmVuIG5lZWQNCj4gPiA+Pj4+PiB0byBoYXZlIHRoaXMgcHJvcGVydHkgaW4g dGhlIERUIGF0IGFsbC4gSXNuJ3QgdGhpcyBhIHByb3BlcnR5DQo+ID4gPj4+Pj4gdGhhdCBpcyBm aXhlZCBmb3IgYSBzcGVjaWZpYyBzaWxpY29uIGltcGxlbWVudGF0aW9uIG9mIHRoZSBEVw0KPiA+ ID4+Pj4+IGNvcmU/IEluIHRoYXQgY2FzZSB3ZSBjb3VsZCBqdXN0IGluZmVyIHRoZSBudW1iZXIg b2YgQVRVcyBmcm9tDQo+ID4gPj4+Pj4gdGhlIERUIGNvbXBhdGlibGUsIHNvIHRoaXMgc2hvdWxk IHByb2JhYmx5IGp1c3QgYmUgYWRkZWQgdG8NCj4gPiA+Pj4+PiBzdHJ1Y3QgcGNpZV9wb3J0IGFu ZCBwcm9wZXJseSBpbml0aWFsaXplZCBieSB0aGUgU29DIGdsdWUgZHJpdmVycy4NCj4gPiA+Pj4+ IFtNaW5naHVhbl0gIEFzIGZhciBhcyBJIGtub3csIGV4eW5vcyBpbXBsZW1lbnRzIG9ubHkgMiBB VFVzLCB0aGlzDQo+ID4gPj4+PiBpcyB3aHkgcGNpZS1kZXNpZ253YXJlIG9ubHkgc3VwcG9ydHMg MiBBVFUuIGlNWCBpbXBsZW1lbnRzIDQgQVRVcw0KPiA+ID4+Pj4gYW5kIExTMTAyMUEgaW1wbGVt ZW50cyA2IEFUVXMuDQo+ID4gPj4+Pg0KPiA+ID4+PiBSaWdodCBzbyB3ZSBkb24ndCBuZWVkIGFu IGFkZGl0aW9uYWwgcHJvcGVydHkgaW4gdGhlIERUIGF0IGFsbC4NCj4gPiA+Pj4gVGhlIG51bWJl ciBvZiBBVFVzIGlzIGZpeGVkIGZvciBhIHNwZWNpZmljIGNvcmUgY29tcGF0aWJsZSBhbmQgY2Fu DQo+ID4gPj4+IGJlIHBhc3NlZCBpbiBieSB0aGUgcmVzcGVjdGl2ZSBleHlub3MsIGlteCBhbmQg bHMxMDIxIGdsdWUgZHJpdmVycy4NCj4gPiA+Pj4NCj4gPiA+Pj4gWW91IG1heSBhc2sgdGhlIEtl eXN0b25lIGFuZCBTcGVhciBtYWludGFpbmVycyB0byBnZXQgdGhlIGNvcnJlY3QNCj4gPiA+Pj4g bnVtYmVyIG9mIEFUVXMgZm9yIHRob3NlIGltcGxlbWVudGF0aW9ucy4NCiA+ID4+Pg0KPiA+ID4+ PiBSZWdhcmRzLA0KPiA+ID4+PiBMdWNhcw0KPiA+ID4+IFtNaW5naHVhbl0gWWVzLiBUaGlzIGEg d2F5IHRoYXQgc3BlY2lmaWMgY29yZSBkcml2ZXIgcGFzc2VzIHRoZSBBVFUNCj4gPiA+PiBudW1i ZXIgdG8gcGNpLWRlc2lnbndhcmUuIEJ1dCBJIHBlcmZlciB0byBhZGRpbmcgZHRzIG5vZGUgZm9y IHRoZQ0KPiA+ID4+IGZvbGxvd2luZyByZWFzb25zOg0KPiA+ID4+IDEuIEFUVSBudW1iZXIgaXMg aGFyZHdhcmUgYXR0cmlidXRlLCBzbyBpdCBjYW4gYmUgYWRkZWQgdG8gRFRTLg0KPiA+ID4gQnV0 IGl0IGlzIGEgZHVwbGljYXRpb24gb2YgaW5mb3JtYXRpb24gdGhhdCBjYW4gYmUgaW5mZXJyZWQg ZnJvbSB0aGUNCj4gPiA+IERUIGNvbXBhdGlibGUgYWxvbmUsIHdoaWNoIGlzIHVzdWFsbHkgZnJv d25lZCB1cG9uLg0KPiA+ID4NCj4gPiA+IEFsc28gaW4gY29udHJhc3QgdG8gdGhlIG51bS1sYW5l cyBwcm9wZXJ0eSBJIGRvbid0IHNlZSBhIHVzZS1jYXNlIHRvDQo+ID4gPiByZWR1Y2UgdGhlIG51 bWJlciBvZiB1c2VkIEFUVXMgaW4gYSBzcGVjaWZpYyBzeXN0ZW0sIHNvIG51bS1hdHVzIGlzDQo+ ID4gPiBiYXNpY2FsbHkgZml4ZWQgZm9yIGEgc3BlY2lmaWMgaW1wbGVtZW50YXRpb24uDQo+ID4g W01pbmdodWFuXSA0QVRVcyBwcm92aWRlcyBiZXR0ZXIgc3VwcG9ydCBmb3IgbXVsdGlwbGUtZnVu Y3Rpb24gYW5kDQo+ID4gU1ItSU9WIFBDSWUgZGV2aWNlcy4NCj4gPiBDYW4gYW55b25lIHRlbGwg bWUgaWYgdGhlcmUgaXMgdGhlIGNvbXBhbnkgd2lsbCBpbmNyZWFzZSBBVFVzIG51bWJlcj8NCj4g PiBJZiB5ZXMsIHdlIHNob3VsZCBhdm9pZCB0aGUgZm9sbG93aW5nIGNvZGU6DQo+ID4NCj4gPiBp ZiAob2ZfZGV2aWNlX2lzX2NvbXBhdGlibGUocGNpZS0+ZGV2LT5vZl9ub2RlLCAieHh4LHIxLXBj aWUiKSkNCj4gPiAgICAgIGF0dV9udW0gPSAyOw0KPiA+DQo+ID4gaWYgKG9mX2RldmljZV9pc19j b21wYXRpYmxlKHBjaWUtPmRldi0+b2Zfbm9kZSwgInh4eCxyMi1wY2llIikpDQo+ID4gICAgICBh dHVfbnVtID0gNDsNCj4gPg0KPiBUaGUgbnVtYmVyIG9mIEFUVXMgaXMgZml4ZWQgZm9yIGEgc3Bl Y2lmaWMgaW1wbGVtZW50YXRpb24uIElmIHRoZSBudW1iZXINCj4gb2YgQVRVcyBjaGFuZ2VzIGlu IGEgbmV3ZXIgaW1wbGVtZW50YXRpb24gb2YgYSBTb0MgdGhlbiB0aGUgc2lsaWNvbg0KPiBpbXBs ZW1lbnRhdGlvbiBvZiB0aGUgRFcgUENJZSBjb3JlIGNoYW5nZWQsIHNvIGl0IHNob3VsZCBnZXQg YSBuZXcNCj4gY29tcGF0aWJsZSBhbnl3YXkuDQo+IA0KDQpJZiB0aGVyZSBhcmUgbXVsdGlwbGUg cGxhdGZvcm1zIHRvIHVzZSB0aGUgc2FtZSBTb0MgZHJpdmVyLCB0aGVyZSB3aWxsIGJlDQptdWx0 aXBsZSBjYXNlcyBmb3IgZGlmZmVyZW50IHBsYXRmb3JtcyBhcyBNaW5naHVhbiBoYXMgcG9pbnRl ZCBvdXQuDQoNCkZvciB0aGUgcGxhdGZvcm0gdGhhdCBvbmx5IGhhcyAyIEFUVXMsIEFUVTAgaXMg dXNlZCBmb3IgQ0ZHMCBhbmQgTUVNIGFuZA0KdGhlcmUgd2lsbCBiZSBjb25mbGljdCB3aGVuIE1F TSBhbmQgQ0ZHMCBhcmUgYWNjZXNzZWQgc2ltdWx0YW5lb3VzbHkgd2hpY2gNCmhhcyBjYXVzZSB0 aGUgU0FUQSBsaW5rIGlzc3VlLiBJIHRoaW5rIG1vcmUgQVRVcyB3aWxsIGJlIGFkZGVkIGluIHRo ZSANCmZ1dHVyZSBkZXNpZ24sIHRoZW4gb25lIGRyaXZlciBtYXliZSBuZWVkIHRvIGhhbmRsZSBt dWx0aXBsZSBwbGF0Zm9ybXMuDQoNCkRUUyBpcyBkZXNpZ25lZCBmb3IgdG8gZGVzY3JpYmUgdGhl IGhhcmR3YXJlIHByb3BlcnR5LCBpdCdzIGEgZ2VuZXJhbCB3YXkNCnRvIHB1dCBoYXJkd2FyZSBy ZWxhdGVkIHByb3BlcnR5IGludG8gRFRTLg0KDQpUaGFua3MsDQpNaW5na2FpDQoNCg== ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-14 11:30 ` Mingkai.Hu @ 2014-11-14 11:42 ` Lucas Stach 2014-11-17 2:58 ` Lian Minghuan-B31939 0 siblings, 1 reply; 18+ messages in thread From: Lucas Stach @ 2014-11-14 11:42 UTC (permalink / raw) To: Mingkai.Hu@freescale.com Cc: Minghuan.Lian@freescale.com, linux-pci@vger.kernel.org, Srikanth Thokala, Bjorn Helgaas, Roy Zang, linux-arm-kernel@lists.infradead.org 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-14 11:42 ` Lucas Stach @ 2014-11-17 2:58 ` Lian Minghuan-B31939 2014-11-17 10:25 ` Lucas Stach 0 siblings, 1 reply; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-17 2:58 UTC (permalink / raw) To: Lucas Stach, Mingkai.Hu@freescale.com Cc: Minghuan.Lian@freescale.com, linux-pci@vger.kernel.org, Srikanth Thokala, Bjorn Helgaas, Roy Zang, linux-arm-kernel@lists.infradead.org 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-17 2:58 ` Lian Minghuan-B31939 @ 2014-11-17 10:25 ` Lucas Stach 0 siblings, 0 replies; 18+ messages in thread From: Lucas Stach @ 2014-11-17 10:25 UTC (permalink / raw) To: Lian Minghuan-B31939, Grant Likely, Rob Herring, devicetree, Mark Rutland, Kumar Gala, Ian Campbell Cc: Mingkai.Hu@freescale.com, linux-pci@vger.kernel.org, Minghuan.Lian@freescale.com, Srikanth Thokala, Bjorn Helgaas, Roy Zang, linux-arm-kernel@lists.infradead.org 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 -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment 2014-11-12 16:23 ` Srikanth Thokala 2014-11-12 16:32 ` Lucas Stach @ 2014-11-14 9:36 ` Lian Minghuan-B31939 1 sibling, 0 replies; 18+ messages in thread From: Lian Minghuan-B31939 @ 2014-11-14 9:36 UTC (permalink / raw) To: Srikanth Thokala Cc: Minghuan Lian, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Zang Roy-R61911, Hu Mingkai-B21284, Bjorn Helgaas, Lucas Stach 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 > >> >> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-11-17 10:25 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-11 5:07 [PATCH v2] PCI: designware: Add support 4 ATUs assignment Minghuan Lian 2014-11-12 6:22 ` Srikanth Thokala 2014-11-12 7:14 ` Lian Minghuan-B31939 2014-11-12 9:01 ` Srikanth Thokala 2014-11-12 10:09 ` Lian Minghuan-B31939 2014-11-12 16:23 ` Srikanth Thokala 2014-11-12 16:32 ` Lucas Stach 2014-11-13 10:02 ` Lian Minghuan-B31939 2014-11-13 10:20 ` Lucas Stach 2014-11-13 10:52 ` Lian Minghuan-B31939 2014-11-13 11:09 ` Lucas Stach 2014-11-14 8:47 ` Lian Minghuan-B31939 2014-11-14 10:02 ` Lucas Stach 2014-11-14 11:30 ` Mingkai.Hu 2014-11-14 11:42 ` Lucas Stach 2014-11-17 2:58 ` Lian Minghuan-B31939 2014-11-17 10:25 ` Lucas Stach 2014-11-14 9:36 ` Lian Minghuan-B31939
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).