Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base
Bruce Griffith wrote: > Replicated code in the wrappers for CIM-X, AGESA, and pre-AGESA is > less of a concern. That would be at a lower priority than src/mainboards. Yes, I agree. I'm excited about your improvements! //Peter -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base
> This is awesome news. Was it intentional that you did not reply to the > list > too? It was intentional but probably poor judgment. This reply is to the list. > As can be seen from the latest responses this seems to be considered > quite controversial. The focus is on AMD-designed boards, but may impact other boards with AMD processors and chipsets. Marc and Martin are watching over the effort and the patches will be pushed to Gerrit where they will go through the normal review process. > My message was also about code outside `src/mainboard`. But let’s take one > step at a time. Replicated code in the wrappers for CIM-X, AGESA, and pre-AGESA is less of a concern. That would be at a lower priority than src/mainboards. > -Original Message- > From: Paul Menzel [mailto:paulepan...@users.sourceforge.net] > Sent: 07 March 2013 4:35 AM > To: Bruce Griffith > Subject: Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base > > Dear Bruce, > > thank you very much for your response and sorry for my late reply. > > Am Montag, den 04.03.2013, 18:40 -0600 schrieb Bruce Griffith: > > > You're right. There's a lot of replicated code in the AMD-specific > > parts of the coreboot tree. In the past, the focus has been to get > > new parts and development boards released as early as possible. And > > that has been at the expense of keeping the codebase clean. > > > > There is an effort starting up to clean up the codebase to [get] as much > > code as possible out of the mainboards directories for AMD boards. As > > a consequence, we expect that some amount of code will get refactored > > into common files. Using standard file names, function names, and > > variable names when similar code needs to be differentiated by > > processor or chipset is a good suggestion. > > > > I expect that we will start to have patches into gerrit within a week > > or two. > > This is awesome news. Was it intentional that you did not reply to the > list > too? As can be seen from the latest responses this seems to be considered > quite controversial. > > My message was also about code outside `src/mainboard`. But let’s take one > step at a time. > > Thanks, > Paul -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base
David Hendricks wrote: > I suspect it's best to simply accept the ugly parts rather than > diverge from whatever AMD uses internally. I think it depends. If the coreboot repo is basically a write-once medium for the AMD code and if what gets written never has any reuse anyway then I think it would be fine to do cleanup. ron minnich wrote: > Unless you're prepared to test and verify on the exact hardware, > don't do it. I agree with this. Testing is important after any major change. I don't think it matters if we have life more complicated than the kernel, this particular cleanup effort would be quite isolated and can take place in some separate branch maintained by Paul or someone else. It's not a high priority, but I do think it would be nice to have. If at some point there is a perfectly clear branch with some simple commits that unify syntactical differences then that can be trivially reviewed by a larger group, and also easily tested and merged if there are no problems. This is of course the same model as for all other development, except I think a branch is warranted since it's a string of commits which do different logical changes but very much belong together, so I'd prefer to merge them all at once. > The kernel is not a guide for firmware work. This is true, but a good structure is nothing unique to either kernel or firmware work - it's just basic good programming. That said - I think that this particular construction site is not so important, and I would much rather like to see effort spent on some of "our own" construction sites in the code. One example is to streamline code (or just files) across mainboard directories, or any one of the numerous worthwhile http://www.coreboot.org/Infrastructure_Projects //Peter -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base
Unless you're prepared to test and verify on the exact hardware, don't do it. We've had lots of well-intentioned efforts like this over the years that broke platforms. I don't like finding that somebody has broken a platform 2 years after a cosmetic improvement of this sort was made. And that has happened. Don't do it. This type of merging is almost always a mistake. Kernel code does not do nearly as much complex work as firmware code. Kernels have it easy. That lesson has been learned many times. The kernel is not a guide for firmware work. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] Unifying AMD SB700/SB800/SB900 code base
On Sat, Mar 2, 2013 at 2:07 AM, Paul Menzel <
paulepan...@users.sourceforge.net> wrote:
> Dear coreboot folks,
>
>
> please take a look at the following difference of two files.
>
> $ diff -u src/southbridge/amd/cimx/sb{8,9}00/late.c
> --- src/southbridge/amd/cimx/sb800/late.c 2013-03-02
> 08:52:51.313654244 +0100
> +++ src/southbridge/amd/cimx/sb900/late.c 2013-03-02
> 08:51:52.157327591 +0100
>
>
...
>
> So changes are mainly,
>
> 1. Different capitalization of file names.
> 2. Different function names (sb800 vs. sb900).
> 3. Different comments.
> 4. Changes to the SB800 file which have not been ported to the SB900
> file.
>
> So looking at the lspci output,
>
> 00:14.5 USB controller: Advanced Micro Devices [AMD] nee ATI
> SB7x0/SB8x0/SB9x0 USB OHCI2 Controller
>
> Linux drivers(?) seem to use the same code for SB7x0/SB8x0/SB9x0 too. So
> I just assume that they are indeed very similar.
>
> Should not we do the same? Especially because of item 4., that fixes get
> ported to all generations.
>
> If that is unfeasible at least spelling of filenames should be kept
> consistent in my opinion.
>
Those seem to be from AGESA/CIMX. For better or worse, it models the
vendor's internal code development processes so that coreboot gets code
drops like the commercial BIOS vendors. I guess AMD developed the SB900
separately from the SB800 to utilize chipset-specific debug features, and
then sanitized the code prior to releasing it.
You're right to point out the problems with this model, but in general I
suspect it's best to simply accept the ugly parts rather than diverge from
whatever AMD uses internally.
For more info about AGESA/CIMX and coreboot, see
http://blogs.amd.com/work/2011/02/28/technical-details-coreboot/
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
