Kerry Sheh (shekairui@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/206
-gerrit
commit 7f2d27b0bddf97948fff2f81fab52633e0f77709 Author: Kerry She shekairui@gmail.com Date: Tue Sep 13 18:29:27 2011 +0800
rs780: hide unused gpp ports
hide unused gpp ports, test on avalue/eax-785e
Change-Id: Iaabfd362a0a01f21d0f49aa2bd2d26f9259013fb Signed-off-by: Kerry She kerry.she@amd.com Signed-off-by: Kerry She shekairui@gmail.com --- src/southbridge/amd/rs780/gfx.c | 2 ++ src/southbridge/amd/rs780/pcie.c | 13 +++++++++++++ src/southbridge/amd/rs780/rs780.c | 5 ++++- src/southbridge/amd/rs780/rs780.h | 1 + 4 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/src/southbridge/amd/rs780/gfx.c b/src/southbridge/amd/rs780/gfx.c index 9262bb9..3c06d44 100644 --- a/src/southbridge/amd/rs780/gfx.c +++ b/src/southbridge/amd/rs780/gfx.c @@ -1009,6 +1009,7 @@ static void single_port_configuration(device_t nb_dev, device_t dev) set_nbmisc_enable_bits(nb_dev, 0x7, 1 << 3, 1 << 3); } } else { /* step 13.b Link Training was successful */ + AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */ set_pcie_enable_bits(dev, 0xA2, 0xFF, 0x1); reg32 = nbpcie_p_read_index(dev, 0x29); width = reg32 & 0xFF; @@ -1064,6 +1065,7 @@ static void dual_port_configuration(device_t nb_dev, device_t dev) set_nbmisc_enable_bits(nb_dev, 0x0c, 1 << dev_ind, 1 << dev_ind);
} else { /* step 16.b Link Training was successful */ + AtiPcieCfg.PortDetect |= 1 << dev_ind; reg32 = nbpcie_p_read_index(dev, 0xa2); width = (reg32 >> 4) & 0x7; printk(BIOS_DEBUG, "GFX LC_LINK_WIDTH = 0x%x.\n", width); diff --git a/src/southbridge/amd/rs780/pcie.c b/src/southbridge/amd/rs780/pcie.c index 9cbd832..5e2d985 100644 --- a/src/southbridge/amd/rs780/pcie.c +++ b/src/southbridge/amd/rs780/pcie.c @@ -390,3 +390,16 @@ void config_gpp_core(device_t nb_dev, device_t sb_dev) switching_gpp_configurations(nb_dev, sb_dev); ValidatePortEn(nb_dev); } + +/** + * Hide unused Gpp port + */ +void pcie_hide_unused_ports(device_t nb_dev) +{ + u16 hide = 0x6FC; /* skip port 0, 1, 8 */ + + hide &= ~(AtiPcieCfg.PortDetect | AtiPcieCfg.PortHp); + printk(BIOS_INFO, "rs780 unused GPP ports bitmap=0x%03x, force disabled\n", hide); + set_nbmisc_enable_bits(nb_dev, 0x0C, 0xFC, (hide & 0xFC)); /* bridge 2-7 */ + set_nbmisc_enable_bits(nb_dev, 0x0C, 0x30000, ((hide >> 9) & 0x3) << 16); /* bridge 9-a */ +} diff --git a/src/southbridge/amd/rs780/rs780.c b/src/southbridge/amd/rs780/rs780.c index d8f1be3..b8c7d04 100644 --- a/src/southbridge/amd/rs780/rs780.c +++ b/src/southbridge/amd/rs780/rs780.c @@ -362,7 +362,10 @@ void rs780_enable(device_t dev) if (dev->enabled) rs780_gpp_sb_init(nb_dev, dev, dev_ind);
- if (dev_ind == 10) disable_pcie_bar3(nb_dev); + if (dev_ind == 10) { + disable_pcie_bar3(nb_dev); + pcie_hide_unused_ports(nb_dev); + } break; default: printk(BIOS_DEBUG, "unknown dev: %s\n", dev_path(dev)); diff --git a/src/southbridge/amd/rs780/rs780.h b/src/southbridge/amd/rs780/rs780.h index aba3e69..5b8d251 100644 --- a/src/southbridge/amd/rs780/rs780.h +++ b/src/southbridge/amd/rs780/rs780.h @@ -213,4 +213,5 @@ u32 extractbits(u32 source, int lsb, int msb); int cpuidFamily(void); int is_family0Fh(void); int is_family10h(void); +void pcie_hide_unused_ports(device_t nb_dev); #endif /* RS780_H */
Dear Kerry,
Am Dienstag, den 13.09.2011, 12:08 +0200 schrieb Kerry Sheh:
Kerry Sheh (shekairui@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/206
is there a typo in your Gerrit user information: »Sheh«?
-gerrit
commit 7f2d27b0bddf97948fff2f81fab52633e0f77709 Author: Kerry She shekairui@gmail.com Date: Tue Sep 13 18:29:27 2011 +0800
rs780: hide unused gpp ports hide unused gpp ports, test on avalue/eax-785e
Copying the commit summary into the commit message body does not have any benefits.
Could you update the commit message using `git commit --amend`?
1. Why is hiding these ports needed? Did you get an error message? 2. Looking at the code, does this patch do more than is written in the commit message? What does for example `AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */` do? Is that also hiding these ports? (I am sorry for this noob question.) If this is doing something else please add that to the commit message or even better split the commit up to make it even smaller.
Thanks,
Paul
Change-Id: Iaabfd362a0a01f21d0f49aa2bd2d26f9259013fb Signed-off-by: Kerry She <kerry.she@amd.com> Signed-off-by: Kerry She <shekairui@gmail.com>
src/southbridge/amd/rs780/gfx.c | 2 ++ src/southbridge/amd/rs780/pcie.c | 13 +++++++++++++ src/southbridge/amd/rs780/rs780.c | 5 ++++- src/southbridge/amd/rs780/rs780.h | 1 + 4 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/src/southbridge/amd/rs780/gfx.c b/src/southbridge/amd/rs780/gfx.c index 9262bb9..3c06d44 100644 --- a/src/southbridge/amd/rs780/gfx.c +++ b/src/southbridge/amd/rs780/gfx.c @@ -1009,6 +1009,7 @@ static void single_port_configuration(device_t nb_dev, device_t dev) set_nbmisc_enable_bits(nb_dev, 0x7, 1 << 3, 1 << 3); } } else { /* step 13.b Link Training was successful */
AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */@@ -1064,6 +1065,7 @@ static void dual_port_configuration(device_t nb_dev, device_t dev) set_nbmisc_enable_bits(nb_dev, 0x0c, 1 << dev_ind, 1 << dev_ind);
} else { /* step 16.b Link Training was successful */
AtiPcieCfg.PortDetect |= 1 << dev_ind;diff --git a/src/southbridge/amd/rs780/pcie.c b/src/southbridge/amd/rs780/pcie.c index 9cbd832..5e2d985 100644 --- a/src/southbridge/amd/rs780/pcie.c +++ b/src/southbridge/amd/rs780/pcie.c @@ -390,3 +390,16 @@ void config_gpp_core(device_t nb_dev, device_t sb_dev) switching_gpp_configurations(nb_dev, sb_dev); ValidatePortEn(nb_dev); }
+/**
- Hide unused Gpp port
- */
+void pcie_hide_unused_ports(device_t nb_dev) +{
- u16 hide = 0x6FC; /* skip port 0, 1, 8 */
- hide &= ~(AtiPcieCfg.PortDetect | AtiPcieCfg.PortHp);
- printk(BIOS_INFO, "rs780 unused GPP ports bitmap=0x%03x, force disabled\n", hide);
- set_nbmisc_enable_bits(nb_dev, 0x0C, 0xFC, (hide & 0xFC)); /* bridge 2-7 */
- set_nbmisc_enable_bits(nb_dev, 0x0C, 0x30000, ((hide >> 9) & 0x3) << 16); /* bridge 9-a */
+} diff --git a/src/southbridge/amd/rs780/rs780.c b/src/southbridge/amd/rs780/rs780.c index d8f1be3..b8c7d04 100644 --- a/src/southbridge/amd/rs780/rs780.c +++ b/src/southbridge/amd/rs780/rs780.c @@ -362,7 +362,10 @@ void rs780_enable(device_t dev) if (dev->enabled) rs780_gpp_sb_init(nb_dev, dev, dev_ind);
if (dev_ind == 10) disable_pcie_bar3(nb_dev);
if (dev_ind == 10) {disable_pcie_bar3(nb_dev);pcie_hide_unused_ports(nb_dev);}diff --git a/src/southbridge/amd/rs780/rs780.h b/src/southbridge/amd/rs780/rs780.h index aba3e69..5b8d251 100644 --- a/src/southbridge/amd/rs780/rs780.h +++ b/src/southbridge/amd/rs780/rs780.h @@ -213,4 +213,5 @@ u32 extractbits(u32 source, int lsb, int msb); int cpuidFamily(void); int is_family0Fh(void); int is_family10h(void); +void pcie_hide_unused_ports(device_t nb_dev); #endif /* RS780_H */
Dear Paul
-----Original Message----- From: coreboot-bounces+kerry.she=amd.com@coreboot.org [mailto:coreboot- bounces+kerry.she=amd.com@coreboot.org] On Behalf Of Paul Menzel Sent: Tuesday, September 13, 2011 6:25 PM To: coreboot@coreboot.org Cc: Kerry She Subject: Re: [coreboot] Patch set updated for coreboot: 7f2d27b rs780: hide unused gpp ports
Dear Kerry,
Am Dienstag, den 13.09.2011, 12:08 +0200 schrieb Kerry Sheh:
Kerry Sheh (shekairui@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/206
is there a typo in your Gerrit user information: »Sheh«?
I have update the profile :)
-gerrit
commit 7f2d27b0bddf97948fff2f81fab52633e0f77709 Author: Kerry She shekairui@gmail.com Date: Tue Sep 13 18:29:27 2011 +0800
rs780: hide unused gpp ports hide unused gpp ports, test on avalue/eax-785eCopying the commit summary into the commit message body does not have any benefits.
Could you update the commit message using `git commit --amend`?
- Why is hiding these ports needed? Did you get an error message?
the information get by Lspci -vvv may not accurate if no device on the pcie port, such as the link speed etc.
- Looking at the code, does this patch do more than is written in the
commit message? What does for example `AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */` do? Is that also hiding these ports? (I am sorry for this noob question.) If this is doing something else please add that to the commit message or even better split the commit up to make it even smaller.
This because the pcie training code for gfx port 2 is hardcoded, If the training successful, we make a mark on the PortDect bitmap. I think we will improve the magic number later.
Thanks for your elaborative review. Kerry
Thanks,
Paul
Change-Id: Iaabfd362a0a01f21d0f49aa2bd2d26f9259013fb Signed-off-by: Kerry She <kerry.she@amd.com> Signed-off-by: Kerry She <shekairui@gmail.com>
src/southbridge/amd/rs780/gfx.c | 2 ++ src/southbridge/amd/rs780/pcie.c | 13 +++++++++++++ src/southbridge/amd/rs780/rs780.c | 5 ++++- src/southbridge/amd/rs780/rs780.h | 1 + 4 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/src/southbridge/amd/rs780/gfx.c b/src/southbridge/amd/rs780/gfx.c index 9262bb9..3c06d44 100644 --- a/src/southbridge/amd/rs780/gfx.c +++ b/src/southbridge/amd/rs780/gfx.c @@ -1009,6 +1009,7 @@ static void single_port_configuration(device_t
nb_dev, device_t dev)
set_nbmisc_enable_bits(nb_dev, 0x7, 1 << 3, 1 << 3); }} else { /* step 13.b Link Training was successful */
AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */@@ -1064,6 +1065,7 @@ static void dual_port_configuration(device_t
nb_dev, device_t dev)
set_nbmisc_enable_bits(nb_dev, 0x0c, 1 << dev_ind, 1 <<dev_ind);
} else { /* step 16.b Link Training was successful */
AtiPcieCfg.PortDetect |= 1 << dev_ind;diff
--git a/src/southbridge/amd/rs780/pcie.c b/src/southbridge/amd/rs780/pcie.c index 9cbd832..5e2d985 100644 --- a/src/southbridge/amd/rs780/pcie.c +++ b/src/southbridge/amd/rs780/pcie.c @@ -390,3 +390,16 @@ void config_gpp_core(device_t nb_dev, device_t
sb_dev)
switching_gpp_configurations(nb_dev, sb_dev);ValidatePortEn(nb_dev); }
+/**
- Hide unused Gpp port
- */
+void pcie_hide_unused_ports(device_t nb_dev) {
- u16 hide = 0x6FC; /* skip port 0, 1, 8 */
- hide &= ~(AtiPcieCfg.PortDetect | AtiPcieCfg.PortHp);
- printk(BIOS_INFO, "rs780 unused GPP ports bitmap=0x%03x, force
disabled\n", hide);
- set_nbmisc_enable_bits(nb_dev, 0x0C, 0xFC, (hide & 0xFC)); /*
bridge 2-7 */
- set_nbmisc_enable_bits(nb_dev, 0x0C, 0x30000, ((hide >> 9) & 0x3)
<<
+16); /* bridge 9-a */ } diff --git a/src/southbridge/amd/rs780/rs780.c b/src/southbridge/amd/rs780/rs780.c index d8f1be3..b8c7d04 100644 --- a/src/southbridge/amd/rs780/rs780.c +++ b/src/southbridge/amd/rs780/rs780.c @@ -362,7 +362,10 @@ void rs780_enable(device_t dev) if (dev->enabled) rs780_gpp_sb_init(nb_dev, dev, dev_ind);
if (dev_ind == 10) disable_pcie_bar3(nb_dev);
if (dev_ind == 10) {disable_pcie_bar3(nb_dev);pcie_hide_unused_ports(nb_dev);}--git
a/src/southbridge/amd/rs780/rs780.h b/src/southbridge/amd/rs780/rs780.h index aba3e69..5b8d251 100644 --- a/src/southbridge/amd/rs780/rs780.h +++ b/src/southbridge/amd/rs780/rs780.h @@ -213,4 +213,5 @@ u32 extractbits(u32 source, int lsb, int msb); int cpuidFamily(void); int is_family0Fh(void); int is_family10h(void); +void pcie_hide_unused_ports(device_t nb_dev); #endif /* RS780_H */
Dear Kerry,
Am Mittwoch, den 14.09.2011, 10:11 +0800 schrieb She, Kerry:
Sent: Tuesday, September 13, 2011 6:25 PM
Am Dienstag, den 13.09.2011, 12:08 +0200 schrieb Kerry Sheh:
Kerry Sheh (shekairui@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/206
is there a typo in your Gerrit user information: »Sheh«?
I have update the profile :)
I thought it is written without the h (She) as in your AMD address. It looks like Gerrit now added an h to the end everywhere.
-gerrit
commit 7f2d27b0bddf97948fff2f81fab52633e0f77709 Author: Kerry She shekairui@gmail.com Date: Tue Sep 13 18:29:27 2011 +0800
rs780: hide unused gpp ports hide unused gpp ports, test on avalue/eax-785eCopying the commit summary into the commit message body does not have any benefits.
Could you update the commit message using `git commit --amend`?
- Why is hiding these ports needed? Did you get an error message?
the information get by Lspci -vvv may not accurate if no device on the pcie port, such as the link speed etc.
Thank you for the information and updating the commit message. To even be more elaborate a diff of lspci output could also be added next time.
- Looking at the code, does this patch do more than is written in the
commit message? What does for example `AtiPcieCfg.PortDetect |= 1 << 2; /* Port 2 */` do? Is that also hiding these ports? (I am sorry for this noob question.) If this is doing something else please add that to the commit message or even better split the commit up to make it even smaller.
This because the pcie training code for gfx port 2 is hardcoded, If the training successful, we make a mark on the PortDect bitmap. I think we will improve the magic number later.
Thanks again. I do not know if a separate patch for this change would be self-contained. At least it should have been mentioned in the commit message. Mark Brown wrote a blog post about this.
Thanks for your elaborative review.
Thank you for your answer and the patches!
Thanks,
Paul
[1] http://www.sirena.org.uk/log/2011/09/09/making-patches-easy-to-review/
-
Kerry Sheh -
Paul Menzel -
She, Kerry