Re: [coreboot] Patch set updated for coreboot: 7f2d27b rs780: hide unused gpp ports

[She, Kerry]

Dear Paul
> -----Original Message-----
> From: coreboot-bounces+kerry.she=[e-mail] [mailto:coreboot-
> bounces+kerry.she=[e-mail]] On Behalf Of Paul Menzel
> Sent: Tuesday, September 13, 2011 6:25 PM
> To: [e-mail]
> 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 ([e-mail]) 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 
> > 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`?
> 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 <[e-mail]>
> > Signed-off-by: Kerry She <[e-mail]>
> >
> > 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 */