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

[Paul Menzel]

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 ([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 :)

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 
> > > 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.

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/