PCI: designware: Remove unnecessary RC BAR setting

PATCH In review Jingoo Han 6 participants 10 messages Apr 1, 2014 – Apr 25, 2014 PCI

Share & tools

Jingoo Han

author

2014-04-01 10:30 UTC #0 lore raw

PATCH 1/1 PCI: designware: Remove unnecessary RC BAR setting

According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.

Signed-off-by Jingoo Han <[email protected]>

---
Tested on Exynos5440.

 drivers/pci/host/pcie-designware.c |    4 ----
 1 file changed, 4 deletions(-)

drivers/pci/host/pcie-designware.c +0 -4

index 6d23d8c..7bee01f 100644
@@ -798,10 +798,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	/* setup RC BARs */
-	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
-	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
-
 	/* setup interrupt pins */
 	dw_pcie_readl_rc(pp, PCI_INTERRUPT_LINE, &val);
 	val &= 0xffff00ff;

Signature · 1 line

1.7.10.4

Mohit KUMAR DCG

2014-04-02 04:57 UTC #1 lore raw

Hello Jingoo,

Quoted 8 lines

-----Original Message-----
From: Jingoo Han [mailto:[email protected]]
Sent: Tuesday, April 01, 2014 4:00 PM
To: 'Bjorn Helgaas'
Cc: [email protected]; Mohit KUMAR DCG; Pratyush ANAND; 'Marek Vasut'; 'Richard Zhu'; 'Kishon Vijay Abraham I'; 'Jingoo Han'
Subject: [PATCH] PCI: designware: Remove unnecessary RC BAR setting

Quoted 6 lines

According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already.
So, remove unnecessary RC BAR setting.

- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to the BAR that needs to be accessed through link.

Otherwise we can ignore BARs setup here.

Thanks
Mohit

Quoted 26 lines

Signed-off-by Jingoo Han <[email protected]>
---
Tested on Exynos5440.
 drivers/pci/host/pcie-designware.c |    4 ----
 1 file changed, 4 deletions(-)
drivers/pci/host/pcie- renamed from drivers/pci/host/pcie-designware.c +0 -0
designware.c
index 6d23d8c..7bee01f 100644
drivers/pci/host/pcie-designware.c +0 -5
@@ -798,10 +798,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	/* setup RC BARs */
-	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
-	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
-
 	/* setup interrupt pins */
 	dw_pcie_readl_rc(pp, PCI_INTERRUPT_LINE, &val);
 	val &= 0xffff00ff;
--
1.7.10.4

Jingoo Han

author

2014-04-02 05:22 UTC #2 lore raw

Quoted 8 lines · Mohit KUMAR DCG

Quoted 7 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already.
So, remove unnecessary RC BAR setting.

Quoted 4 lines

- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to the BAR that needs to be accessed through link.
Otherwise we can ignore BARs setup here.

Hi Mohit KUMAR DCG,

Thank you for your feedback.

I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.

Best regards,
Jingoo Han

Quoted 29 lines

Thanks
Mohit
Quoted 25 lines
Signed-off-by Jingoo Han <[email protected]>
---
Tested on Exynos5440.
 drivers/pci/host/pcie-designware.c |    4 ----
 1 file changed, 4 deletions(-)
drivers/pci/host/pcie- renamed from drivers/pci/host/pcie-designware.c +0 -0
designware.c
index 6d23d8c..7bee01f 100644
drivers/pci/host/pcie-designware.c +0 -5
@@ -798,10 +798,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	/* setup RC BARs */
-	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
-	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
-
 	/* setup interrupt pins */
 	dw_pcie_readl_rc(pp, PCI_INTERRUPT_LINE, &val);
 	val &= 0xffff00ff;
--
1.7.10.4

Mohit KUMAR DCG

2014-04-02 05:33 UTC #3 lore raw

Hello Jingoo,

Quoted 23 lines

-----Original Message-----
From: Jingoo Han [mailto:[email protected]]
Sent: Wednesday, April 02, 2014 10:53 AM
To: Mohit KUMAR DCG
Cc: 'Bjorn Helgaas'; [email protected]; Pratyush ANAND; 'Marek Vasut'; 'Richard Zhu'; 'Kishon Vijay Abraham I'
Subject: Re: [PATCH] PCI: designware: Remove unnecessary RC BAR setting
Quoted 10 lines · Mohit KUMAR DCG
Quoted 7 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 2 lines
Otherwise we can ignore BARs setup here.

Quoted 8 lines

Hi Mohit KUMAR DCG,
Thank you for your feedback.
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.

- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.

Regards
Mohit

Quoted 31 lines

Best regards,
Jingoo Han
Quoted 28 lines
Thanks
Mohit
Quoted 24 lines
Signed-off-by Jingoo Han <[email protected]>
---
Tested on Exynos5440.
 drivers/pci/host/pcie-designware.c |    4 ----
 1 file changed, 4 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/host/pcie- designware.c index 6d23d8c..7bee01f 100644
drivers/pci/host/pcie-designware.c +0 -5
@@ -798,10 +798,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	/* setup RC BARs */
-	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
-	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
-
 	/* setup interrupt pins */
 	dw_pcie_readl_rc(pp, PCI_INTERRUPT_LINE, &val);
 	val &= 0xffff00ff;
--
1.7.10.4

Kishon Vijay Abraham I

2014-04-02 05:34 UTC #4 lore raw

Quoted 29 lines · Mohit KUMAR DCG

Hello Jingoo,
Quoted 27 lines
-----Original Message-----
From: Jingoo Han [mailto:[email protected]]
Sent: Wednesday, April 02, 2014 10:53 AM
To: Mohit KUMAR DCG
Cc: 'Bjorn Helgaas'; [email protected]; Pratyush ANAND; 'Marek Vasut'; 'Richard Zhu'; 'Kishon Vijay Abraham I'
Subject: Re: [PATCH] PCI: designware: Remove unnecessary RC BAR setting
Quoted 10 lines · Mohit KUMAR DCG
Quoted 7 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 2 lines
Otherwise we can ignore BARs setup here.
Hi Mohit KUMAR DCG,
Thank you for your feedback.

Quoted 7 lines

Quoted 4 lines
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.

Neither do we in DRA7xx.

Cheers
Kishon

Quoted 36 lines

Regards
Mohit
Quoted 31 lines
Best regards,
Jingoo Han
Quoted 28 lines
Thanks
Mohit
Quoted 24 lines
Signed-off-by Jingoo Han <[email protected]>
---
Tested on Exynos5440.
 drivers/pci/host/pcie-designware.c |    4 ----
 1 file changed, 4 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/host/pcie- designware.c index 6d23d8c..7bee01f 100644
drivers/pci/host/pcie-designware.c +0 -5
@@ -798,10 +798,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL);
 
-	/* setup RC BARs */
-	dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0);
-	dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1);
-
 	/* setup interrupt pins */
 	dw_pcie_readl_rc(pp, PCI_INTERRUPT_LINE, &val);
 	val &= 0xffff00ff;
--
1.7.10.4

Marek Vasut

2014-04-02 10:34 UTC #5 lore raw

Quoted 30 lines · Kishon Vijay Abraham I

Quoted 29 lines · Mohit KUMAR DCG
Hello Jingoo,
Quoted 27 lines
-----Original Message-----
From: Jingoo Han [mailto:[email protected]]
Sent: Wednesday, April 02, 2014 10:53 AM
To: Mohit KUMAR DCG
Cc: 'Bjorn Helgaas'; [email protected]; Pratyush ANAND; 'Marek Vasut'; 'Richard Zhu'; 'Kishon Vijay Abraham I'
Subject: Re: [PATCH] PCI: designware: Remove unnecessary RC BAR setting
Quoted 9 lines · Mohit KUMAR DCG
Quoted 5 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 1 line
Otherwise we can ignore BARs setup here.
Hi Mohit KUMAR DCG,
Thank you for your feedback.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.
Neither do we in DRA7xx.

I suspect that means we should keep the code to make sure the registers are configured correctly, no?

Richard, can you comment on MX6 please ?

Best regards,
Marek Vasut

[email protected]

2014-04-02 11:59 UTC #6 lore raw

Hi Marek:

Quoted 49 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Marek Vasut
Sent: Wednesday, April 02, 2014 6:35 PM
To: Kishon Vijay Abraham I
Cc: Mohit KUMAR DCG; Jingoo Han; 'Bjorn Helgaas'; [email protected]; Pratyush ANAND; Zhu Richard-R65037
Subject: Re: [PATCH] PCI: designware: Remove unnecessary RC BAR setting
Quoted 39 lines · Kishon Vijay Abraham I
Quoted 37 lines · Mohit KUMAR DCG
Hello Jingoo,
Quoted 25 lines
-----Original Message-----
From: Jingoo Han [mailto:[email protected]]
Sent: Wednesday, April 02, 2014 10:53 AM
To: Mohit KUMAR DCG
Cc: 'Bjorn Helgaas'; [email protected]; Pratyush ANAND; 'Marek Vasut'; 'Richard Zhu'; 'Kishon Vijay Abraham I'
Subject: Re: [PATCH] PCI: designware: Remove unnecessary RC BAR setting
Quoted 10 lines · Mohit KUMAR DCG
Quoted 5 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 1 line
Otherwise we can ignore BARs setup here.
Quoted 7 lines
Hi Mohit KUMAR DCG,
Thank you for your feedback.
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.

Quoted 6 lines

Quoted 1 line
Neither do we in DRA7xx.
I suspect that means we should keep the code to make sure the registers are configured correctly, no?
Richard, can you comment on MX6 please ?

[Richard] Sorry to reply late. I'm engaged in another stuff in the past days.
i.MX6 pcie doesn't use RC's BAR0/1 in applications either.

Quoted 8 lines

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

Best Regards
Richard Zhu

Jingoo Han

author

2014-04-04 02:31 UTC #7 lore raw

Quoted 33 lines · Richard Zhu

Quoted 32 lines · Marek Vasut
Quoted 30 lines · Kishon Vijay Abraham I
Quoted 19 lines · Mohit KUMAR DCG
Quoted 18 lines · Jingoo Han
Quoted 10 lines · Mohit KUMAR DCG
Quoted 5 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 1 line
Otherwise we can ignore BARs setup here.
Hi Mohit KUMAR DCG,
Quoted 8 lines
Quoted 5 lines
Thank you for your feedback.
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.
Neither do we in DRA7xx.

Quoted 8 lines

Quoted 4 lines
I suspect that means we should keep the code to make sure the registers are configured correctly, no?
Richard, can you comment on MX6 please ?
[Richard] Sorry to reply late. I'm engaged in another stuff in the past days.
i.MX6 pcie doesn't use RC's BAR0/1 in applications either.
Quoted 1 line

Thank you all for your comments!

I noticed that Synopsys PCIe "Dual mode" can support BAR0/BAR1. The bits[3:0] of BAR0 is RO(CS), which means Read-Only, but writable from the local application through the DBI.

So, the BAR0/1 setting might be necessary, in order to ensure the BAR0/1 setting is written properly.

But, when BAR0/1 are implemented, the bits[3:0] of BAR0 is decided by hardware configuration parameters. So, this BAR0/1 setting looks unnecessary.

How about others' opinions?

1. Necessary: in order to ensure the BAR0/BAR1 setting, even though BAR0/BAR1 were already set as hardware default values

2. Unnecessary: the BAR0/BAR1 setting code is dummy, because BAR0/BAR1 were already set as hardware default values

For example, in the case of Exynos5440, BAR0/BAR1 are not implemented; thus, even though BAR0 is written as 0x4, BAR0 can be always read as 0. So, there is not side effect, but just unnecessary code is executed during boot time.

Thank you. :-)

Best regards,
Jingoo Han

Bjorn Helgaas

maintainer

2014-04-24 21:35 UTC #8 lore raw

Quoted 61 lines · Jingoo Han

Quoted 33 lines · Richard Zhu
Quoted 32 lines · Marek Vasut
Quoted 30 lines · Kishon Vijay Abraham I
Quoted 19 lines · Mohit KUMAR DCG
Quoted 18 lines · Jingoo Han
Quoted 10 lines · Mohit KUMAR DCG
Quoted 5 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 1 line
Otherwise we can ignore BARs setup here.
Hi Mohit KUMAR DCG,
Quoted 8 lines
Quoted 5 lines
Thank you for your feedback.
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.
Neither do we in DRA7xx.
Quoted 8 lines
Quoted 4 lines
I suspect that means we should keep the code to make sure the registers are configured correctly, no?
Richard, can you comment on MX6 please ?
[Richard] Sorry to reply late. I'm engaged in another stuff in the past days.
i.MX6 pcie doesn't use RC's BAR0/1 in applications either.
Quoted 1 line
Thank you all for your comments!
I noticed that Synopsys PCIe "Dual mode" can support BAR0/BAR1. The bits[3:0] of BAR0 is RO(CS), which means Read-Only, but writable from the local application through the DBI.
So, the BAR0/1 setting might be necessary, in order to ensure the BAR0/1 setting is written properly.
But, when BAR0/1 are implemented, the bits[3:0] of BAR0 is decided by hardware configuration parameters. So, this BAR0/1 setting looks unnecessary.
How about others' opinions?
1. Necessary: in order to ensure the BAR0/BAR1 setting, even though BAR0/BAR1 were already set as hardware default values

Quoted 7 lines

2. Unnecessary: the BAR0/BAR1 setting code is dummy, because BAR0/BAR1 were already set as hardware default values
For example, in the case of Exynos5440, BAR0/BAR1 are not implemented; thus, even though BAR0 is written as 0x4, BAR0 can be always read as 0. So, there is not side effect, but just unnecessary code is executed during boot time.

I don't sense a clear consensus that we should apply this, so I'll drop it for now. Please repost with appropriate acks if we do need it.

Bjorn

Jingoo Han

author

2014-04-25 01:04 UTC #9 lore raw

Quoted 65 lines · Bjorn Helgaas

Quoted 64 lines · Jingoo Han
Quoted 33 lines · Richard Zhu
Quoted 32 lines · Marek Vasut
Quoted 30 lines · Kishon Vijay Abraham I
Quoted 19 lines · Mohit KUMAR DCG
Quoted 18 lines · Jingoo Han
Quoted 10 lines · Mohit KUMAR DCG
Quoted 5 lines · Jingoo Han
According to the spec, the synopsys core does not implement the optional BARs such as BAR0/1. This is based on the assumption that the RC host probably has registers on some other internal bus and has knowledge and setup access to these registers already. So, remove unnecessary RC BAR setting.
- Normally BARs in RC are not used but somehow available in the design. One possible BAR use can be if RC has some memory connected to
the BAR that needs to be accessed through link.
Quoted 1 line
Otherwise we can ignore BARs setup here.
Hi Mohit KUMAR DCG,
Quoted 8 lines
Quoted 5 lines
Thank you for your feedback.
I want to know whether or not other SoCs such as ST, Freescale, TI support BAR0/BAR1. If no SoC supports BAR0/BAR1, the unnecessary RC BAR setting code should be removed.
- We are not currently using RC's BAR0/1 in any application but no such restriction from HW as ST SoCs support BARs in HW design.
Neither do we in DRA7xx.
Quoted 8 lines
Quoted 4 lines
I suspect that means we should keep the code to make sure the registers are configured correctly, no?
Richard, can you comment on MX6 please ?
[Richard] Sorry to reply late. I'm engaged in another stuff in the past days.
i.MX6 pcie doesn't use RC's BAR0/1 in applications either.
Quoted 1 line
Thank you all for your comments!
I noticed that Synopsys PCIe "Dual mode" can support BAR0/BAR1. The bits[3:0] of BAR0 is RO(CS), which means Read-Only, but writable from the local application through the DBI.
So, the BAR0/1 setting might be necessary, in order to ensure the BAR0/1 setting is written properly.
But, when BAR0/1 are implemented, the bits[3:0] of BAR0 is decided by hardware configuration parameters. So, this BAR0/1 setting looks unnecessary.
How about others' opinions?
1. Necessary: in order to ensure the BAR0/BAR1 setting, even though BAR0/BAR1 were already set as hardware default values
2. Unnecessary: the BAR0/BAR1 setting code is dummy, because BAR0/BAR1 were already set as hardware default values

Quoted 7 lines

Quoted 4 lines
For example, in the case of Exynos5440, BAR0/BAR1 are not implemented; thus, even though BAR0 is written as 0x4, BAR0 can be always read as 0. So, there is not side effect, but just unnecessary code is executed during boot time.
I don't sense a clear consensus that we should apply this, so I'll drop it for now. Please repost with appropriate acks if we do need it.

OK, I agree with your opinion.
Thank you.

Best regards,
Jingoo Han