* [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-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
* 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
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).