PCI: designware: Add support 4 ATUs assignment
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
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.
---
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(-)
Documentation/devicetree/bindings/pci/designware-pcie.txt +1 -0
@@ -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.
drivers/pci/host/pcie-designware.c +40 -13
121 lines not on this page · raw message
drivers/pci/host/pcie-designware.h +9 -0
@@ -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 {
Signature · 1 line
1.9.1
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi,
Quoted 44 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +1
-0@@ -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.drivers/pci/host/pcie-designware.c +2
-0@@ -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
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 5 lines
drivers/pci/host/pcie-designware.c +1
-1@@ -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?
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 491
Quoted 8 lines
drivers/pci/host/pcie-designware.c +1
-0const __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?
Quoted 98 lines
drivers/pci/host/pcie-designware.c +32
-12+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;
drivers/pci/host/pcie-designware.c void dw_pcie_setup_rc(struct pcie_port *pp) · line 799
Quoted 7 lines
drivers/pci/host/pcie-designware.c +4
-0@@ -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
Quoted 37 lines
drivers/pci/host/pcie-designware.c +0
-0/* set the number of lanes */ dw_pcie_readl_rc (pp,PCIE_PORT_LINK_CONTROL , &val); val &= ~PORT_LINK_MODE_MASK ;drivers/pci/host/pcie-designware.h +9
-1@@ -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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 46 lines · Srikanth Thokala
Hi,
Quoted 42 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +1
-0@@ -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.drivers/pci/host/pcie-designware.c +2
-0@@ -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)
Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 7 lines
drivers/pci/host/pcie-designware.c +1
-1#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.
Quoted 9 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 491
Quoted 8 lines
drivers/pci/host/pcie-designware.c +1
-0const __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.
Quoted 101 lines
Quoted 101 lines
drivers/pci/host/pcie-designware.c +32
-12+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;
Quoted 7 lines
drivers/pci/host/pcie-designware.c void dw_pcie_setup_rc(struct pcie_port *pp) · line 799
Quoted 4 lines
drivers/pci/host/pcie-designware.c +4
-0+/* 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.
Quoted 39 lines
- Srikanth
Quoted 37 lines
drivers/pci/host/pcie-designware.c +0
-0/* set the number of lanes */ dw_pcie_readl_rc (pp,PCIE_PORT_LINK_CONTROL , &val); val &= ~PORT_LINK_MODE_MASK ;drivers/pci/host/pcie-designware.h +9
-1@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Minghuan,
Quoted 69 lines · On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 54 lines · Srikanth Thokala
Hi,
Quoted 49 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +0
-0@@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for newdevicetrees 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.drivers/pci/host/pcie-designware.c +2
-0@@ -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 0x904Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 6 lines
drivers/pci/host/pcie-designware.c +1
-1@@ -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?
Quoted 7 lines
[Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs)
So,num_atusshould be u32 type.
If we use u8 type to definenum_atus, the dts node should be changed tonum_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.
Quoted 149 lines
Quoted 11 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 491
Quoted 9 lines
drivers/pci/host/pcie-designware.c +1
-0const __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.
Quoted 125 lines
Quoted 125 lines
drivers/pci/host/pcie-designware.c +15
-2+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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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])
drivers/pci/host/pcie-designware.c +4
-2+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])
drivers/pci/host/pcie-designware.c +1
-0+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;Quoted 9 lines
drivers/pci/host/pcie-designware.c void dw_pcie_setup_rc(struct pcie_port *pp) · line 799
Quoted 4 lines
drivers/pci/host/pcie-designware.c +4
-0+/* 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?
Quoted 5 lines
[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
Quoted 43 lines
Quoted 41 lines
- Srikanth
Quoted 38 lines
drivers/pci/host/pcie-designware.c +0
-0/* set the number of lanes */ dw_pcie_readl_rc (pp,PCIE_PORT_LINK_CONTROL , &val); val &= ~PORT_LINK_MODE_MASK ;drivers/pci/host/pcie-designware.h +9
-1@@ -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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Srikanth,
please see my comments inline.
Thanks,
Minghuan
Quoted 76 lines · Srikanth Thokala
Hi Minghuan,
Quoted 65 lines · On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 51 lines · Srikanth Thokala
Hi,
Quoted 47 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +0
-0@@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for newdevicetrees 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.drivers/pci/host/pcie-designware.c +2
-0@@ -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)Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 7 lines
drivers/pci/host/pcie-designware.c +1
-1#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?
Quoted 7 lines
[Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs)
So,num_atusshould be u32 type.
If we use u8 type to definenum_atus, the dts node should be changed tonum_atus= /bits/ 8 <8>.
I prefer the previous definition num-atus = <6> which is more simple and clear.
Quoted 2 lines
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>;
Quoted 144 lines
Quoted 144 lines
Quoted 9 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 491
Quoted 8 lines
drivers/pci/host/pcie-designware.c +1
-0const __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.
Quoted 133 lines
Quoted 122 lines
drivers/pci/host/pcie-designware.c +15
-2+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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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])
drivers/pci/host/pcie-designware.c +4
-2+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])
drivers/pci/host/pcie-designware.c +1
-0+dw_pcie_prog_viewport_io_outbound (pp); }return ret;drivers/pci/host/pcie-designware.c void dw_pcie_setup_rc(struct pcie_port *pp) · line 799
Quoted 7 lines
drivers/pci/host/pcie-designware.c +4
-0@@ -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?
Quoted 8 lines
Quoted 5 lines
[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
Quoted 41 lines
Quoted 41 lines
Quoted 40 lines
- Srikanth
Quoted 38 lines
drivers/pci/host/pcie-designware.c +0
-0/* set the number of lanes */ dw_pcie_readl_rc (pp,PCIE_PORT_LINK_CONTROL , &val); val &= ~PORT_LINK_MODE_MASK ;drivers/pci/host/pcie-designware.h +9
-1@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Minghuan,
Quoted 95 lines · On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
please see my comments inline.
Thanks,
MinghuanQuoted 83 lines · Srikanth Thokala
Hi Minghuan,
Quoted 70 lines · On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 54 lines · Srikanth Thokala
Hi,
Quoted 49 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +0
-0@@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for newdevicetrees 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.drivers/pci/host/pcie-designware.c +2
-0@@ -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 0x904Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 6 lines
drivers/pci/host/pcie-designware.c +1
-1@@ -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?
Quoted 7 lines
[Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs)
So,num_atusshould be u32 type.
If we use u8 type to definenum_atus, the dts node should be changed tonum_atus= /bits/ 8 <8>.
I prefer the previous definition num-atus = <6> which is more simple and clear.Quoted 2 lines
Yes, I agree. But as it holds only 6 possible distinct values, I prefer to use u8.
Quoted 6 lines
[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.
Quoted 178 lines
for example:
#address-cells = <3>; #size-cells = <2>; num-lanes = <1>;Quoted 159 lines
Quoted 153 lines
Quoted 10 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 491
Quoted 8 lines
drivers/pci/host/pcie-designware.c +1
-0const __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.
Quoted 128 lines
Quoted 128 lines
drivers/pci/host/pcie-designware.c +15
-2+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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +3
-2@@ -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);drivers/pci/host/pcie-designware.c +0
-0@@ -557,8 +575,9 @@ static voiddw_pcie_prog_viewport_mem_outbound(struct
pcie_port*pp)static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) {drivers/pci/host/pcie-designware.c +0
-2-/* Program viewport 1 : OUTBOUND : IO */ - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND |
PCIE_ATU_REGION_INDEX1,drivers/pci/host/pcie-designware.c +3
-0+/* 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);drivers/pci/host/pcie-designware.c +3
-2@@ -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])
drivers/pci/host/pcie-designware.c +4
-2+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])
drivers/pci/host/pcie-designware.c +1
-0+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;Quoted 11 lines
drivers/pci/host/pcie-designware.c void dw_pcie_setup_rc(struct pcie_port *pp) · line 799
Quoted 4 lines
drivers/pci/host/pcie-designware.c +4
-0+/* 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?Quoted 5 lines
[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.Quoted 3 lines
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 codedrivers/pci/host/pcie-designware.c +4
-0+/* set ATUs setting for MEM and IO */ +dw_pcie_prog_viewport_mem_outbound (pp); +dw_pcie_prog_viewport_io_outbound (pp); +
Quoted 3 lines
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?
Quoted 1 line
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?
Quoted 2 lines
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
Quoted 48 lines
- Srikanth
Quoted 42 lines
Quoted 42 lines
Quoted 40 lines
- Srikanth
Quoted 38 lines
drivers/pci/host/pcie-designware.c +0
-0/* set the number of lanes */ dw_pcie_readl_rc (pp,PCIE_PORT_LINK_CONTROL , &val); val &= ~PORT_LINK_MODE_MASK ;drivers/pci/host/pcie-designware.h +9
-1@@ -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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 104 lines
Hi Minghuan,
Quoted 100 lines · On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
please see my comments inline.
Thanks,
MinghuanQuoted 83 lines · Srikanth Thokala
Hi Minghuan,
Quoted 70 lines · On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 54 lines · Srikanth Thokala
Hi,
Quoted 49 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +0
-0@@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for newdevicetrees 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.drivers/pci/host/pcie-designware.c +2
-0@@ -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 0x904Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 6 lines
drivers/pci/host/pcie-designware.c +1
-1@@ -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?
Quoted 7 lines
[Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs)
So,num_atusshould be u32 type.
If we use u8 type to definenum_atus, the dts node should be changed tonum_atus= /bits/ 8 <8>.
I prefer the previous definition num-atus = <6> which is more simple and clear.Quoted 2 lines
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.
Quoted 6 lines
Quoted 1 line
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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Lucas,
Please see my comments inline.
Thanks,
Minghuan
Quoted 106 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 96 lines
Hi Minghuan,
Quoted 92 lines · On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
please see my comments inline.
Thanks,
MinghuanQuoted 76 lines · Srikanth Thokala
Hi Minghuan,
Quoted 65 lines · On Wed, Nov 12, 2014 at 12:44 PM, Lian Minghuan-B31939 <[email protected]>…
Hi Srikanth,
Thanks for your comments, please see my reply inline.
Quoted 51 lines · Srikanth Thokala
Hi,
Quoted 47 lines · Minghuan Lian
Currently,
pcie-designware.conly 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. Ifnum_atusis 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 <[email protected]>---
change log:
v1-v2:
1. add the default value to property num-atus description
2. Use atu_idx[] instead of single values
3. initializenum_atusto 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(-)Documentation/devicetree/bindings/pci/designware-pcie.txt +0
-0@@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for newdevicetrees 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.drivers/pci/host/pcie-designware.c +2
-0@@ -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)Quoted 8 lines
drivers/pci/host/pcie-designware.c int __init dw_pcie_host_init(struct pcie_port *pp) · line 351
Quoted 7 lines
drivers/pci/host/pcie-designware.c +1
-1#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?
Quoted 7 lines
[Minghuan] I define num-atus like this: num-atus = <6> (our controller supports 6 outbound ATUs)
So,num_atusshould be u32 type.
If we use u8 type to definenum_atus, the dts node should be changed tonum_atus= /bits/ 8 <8>.
I prefer the previous definition num-atus = <6> which is more simple and clear.Quoted 2 lines
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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
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_portand 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.
Quoted 3 lines
Regards,
Lucas
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939:
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.
Quoted 4 lines
[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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Lucas,
Quoted 31 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939:
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 5 lines
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
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939:
Quoted 37 lines
Hi Lucas,
Quoted 34 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939:
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Lucas and all,
Quoted 44 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939:
Quoted 37 lines
Hi Lucas,
Quoted 34 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939:
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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;
Quoted 10 lines
Quoted 4 lines
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
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
Quoted 58 lines
Hi Lucas and all,
Quoted 44 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-B31939:
Quoted 37 lines
Hi Lucas,
Quoted 34 lines · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-B31939:
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.Quoted 6 lines
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:
Quoted 6 lines
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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Quoted 75 lines
-----Original Message-----
From: Lucas Stach [mailto:l.stach at pengutronix.de]
Sent: Friday, November 14, 2014 6:02 PM
To: Lian Minghuan-B31939
Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; Bjorn Helgaas
Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignmentAm Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
Quoted 4 lines
Hi Lucas and all,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-
B31939:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Hi Lucas,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-
B31939:
Quoted 53 lines
Quoted 39 lines
Quoted 33 lines
Quoted 32 lines
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.Quoted 6 lines
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;
Quoted 8 lines
Quoted 3 lines
if (
of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie"))atu_num = 4;The number of ATUs is fixed for a specific implementation. If the number of ATUs changes in a newer implementation of a SoC then the silicon implementation of the DW PCIe core changed, so it should get a new compatible anyway.
If there are multiple platforms to use the same SoC driver, there will be multiple cases for different platforms as Minghuan has pointed out.
For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and there will be conflict when MEM and CFG0 are accessed simultaneously which has cause the SATA link issue. I think more ATUs will be added in the future design, then one driver maybe need to handle multiple platforms.
DTS is designed for to describe the hardware property, it's a general way to put hardware related property into DTS.
Thanks,
Mingkai
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Am Freitag, den 14.11.2014, 11:30 +0000 schrieb
Mingkai.Hu at freescale.com:
Quoted 87 lines
Quoted 75 lines
-----Original Message-----
From: Lucas Stach [mailto:l.stach at pengutronix.de]
Sent: Friday, November 14, 2014 6:02 PM
To: Lian Minghuan-B31939
Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; Bjorn Helgaas
Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignmentAm Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
Quoted 4 lines
Hi Lucas and all,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-
B31939:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Hi Lucas,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-
B31939:
Quoted 53 lines
Quoted 39 lines
Quoted 33 lines
Quoted 32 lines
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.Quoted 6 lines
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;Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
Hi Lucas,
Quoted 99 lines · Lucas Stach
Am Freitag, den 14.11.2014, 11:30 +0000 schrieb
[email protected]:Quoted 85 lines
Quoted 74 lines
-----Original Message-----
From: Lucas Stach [mailto:[email protected]]
Sent: Friday, November 14, 2014 6:02 PM
To: Lian Minghuan-B31939
Cc: Srikanth Thokala; Lian Minghuan-B31939; [email protected]; [email protected]; Zang Roy-R61911; Hu Mingkai-B21284; Bjorn Helgaas
Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignmentAm Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
Quoted 4 lines
Hi Lucas and all,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-
B31939:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Hi Lucas,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-
B31939:
Quoted 53 lines
Quoted 39 lines
Quoted 33 lines
Quoted 32 lines
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.Quoted 6 lines
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;Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
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 structtegra_pcie_soc_dataon 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
Quoted 3 lines
Regards,
Lucas
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
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:
Quoted 109 lines
Hi Lucas,
Quoted 99 lines · Lucas Stach
Am Freitag, den 14.11.2014, 11:30 +0000 schrieb
Mingkai.Hu at freescale.com:Quoted 85 lines
Quoted 74 lines
-----Original Message-----
From: Lucas Stach [mailto:l.stach at pengutronix.de]
Sent: Friday, November 14, 2014 6:02 PM
To: Lian Minghuan-B31939
Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; Bjorn Helgaas
Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignmentAm Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
Quoted 4 lines
Hi Lucas and all,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-
B31939:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Hi Lucas,
Quoted 1 line · Lucas Stach
Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-
B31939:
Quoted 53 lines
Quoted 39 lines
Quoted 33 lines
Quoted 32 lines
Quoted 10 lines
Hi Lucas,
Please see my comments inline.
Thanks,
MinghuanQuoted 2 lines · Lucas Stach
Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
Quoted 1 line
Hi Minghuan,
[...]
Quoted 10 lines
Quoted 10 lines
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_portand properly initialized by the SoC glue drivers.Quoted 4 lines
[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.
Quoted 6 lines
Quoted 2 lines
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.Quoted 6 lines
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;Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
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 structtegra_pcie_soc_dataon how to properly abstract properties that are defined by a specific silicon implementation of a core.
Quoted 8 lines
[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.
Quoted 5 lines
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
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/1 PCI: designware: Add support 4 ATUs assignment
In reply to Srikanth ThokalaQuoted 47 lines · Srikanth Thokala
Hi Minghuan,
On Wed, Nov 12, 2014 at 3:39 PM, Lian Minghuan-B31939 <[email protected]> wrote:
[...]return ret;drivers/pci/host/pcie-designware.c +4
-0@@ -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); +Quoted 23 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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?Quoted 8 lines
Quoted 5 lines
[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 codedrivers/pci/host/pcie-designware.c +4
-0+/* set ATUs setting for MEM and IO */ +dw_pcie_prog_viewport_mem_outbound (pp); +dw_pcie_prog_viewport_io_outbound (pp); +Quoted 3 lines
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?Quoted 1 line
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,
Quoted 8 lines
+++
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'.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 4 lines
- Srikanth
Quoted 2 lines