Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34632 )
Change subject: Documentation: Add guidelines for new mainboard ports ......................................................................
Patch Set 4:
(29 comments)
I'm not sure if using a template would be better than suggesting the same procedure used for mainboards: just copy the documentation from another similar mainboard and edit it until it's true for your board.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... File Documentation/getting_started/mainboard_template.md:
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 15: Consumer What's the criteria one should use to choose this?
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 24: * 'EVT early prototype' (Unit is for 'Engineering validation test') : * 'DVT prototype' (Unit is for 'Design verification test')
I'd still rather we didn't request this information in the template. […]
Would "Prototype" be good enough?
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 30: * What's the expected release date?
I can understand your confusion, nevertheless, releasing the hardware specs by pushing source code i […]
I agree with Martin, "What's the expected release date (if public)?" should be good enough.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 42: * Are the blobs part of the coreboot build process? Please do not count the IFD, GbE, ME, et. al. as part of the coreboot build process. It's just more things that could go wrong.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 44: ## Programming In-circuit programming is worth documenting thoroughly. For instance, presence of a protection diode on the flash chip (or flash chips! there can be several) power rail and resistors between the flash chip(s) and the SoC/chipset means in-circuit programming is intended to work.
I have also seen "programming" headers, which are actually intended to be used with a second flash chip, bypassing the on-board flash chip using the HOLD# pin or a logic gate. That header does not mean the board is safe for in-circuit programming.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 47: Where's the programming header? : * Can you provide a picture? Ideally the picture shows the header, and where it is on the board (by giving some context around it)
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 56: BMC BMC or SuperIO or embedded controller
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 59: * Are there 'backup' features, like DualBIOS, enabled? I'd specify that DualBIOS is a Gigabyte-specific feature (it's even patented). I wouldn't say "enabled", though - "present" sounds better.
And no, software-based "backup" features won't work with coreboot.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 77: Does it support USB serial? Is there any board that can do this?
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 78: LVTTL Even I don't know where I'd find that
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 83: schematics schematics and/or boardviews
Most of these are classified as confidential information, though.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 89: W Please add something stating something along the lines of:
"thing X does not work" is as useful as saying nothing at all. Instead, please consider explaining how the actual behavior differs from the desired behavior, e.g.: "plugging an external graphics card causes the fans to spin at full speed". Additionally, it is advisable to document what was attempted to fix the issue, so that others don't suggest/try it again in vain.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... File Documentation/getting_started/new_mainboard_ports.md:
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 3: mainboards singular 'mainboard'
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 3: Mainboard code is placed under `src/mainboards`. It glues the HW components : together by selecting appropiate northbridge, southbridge, on-board HW and : drivers. : : It provides the devicetree.cb, configures GPIOs, audio codecs, a static ACPI : board desciption and allows the romstage code to retrieve the SPD for DRAM : training. I would suggest rewriting this a bit:
Mainboard code, located under `src/mainboard`, specifies how hardware components are connected together on a specific mainboard. Examples include: the devicetree, GPIO settings, audio codec parameters, the ACPI DSDT, memory-related parameters...
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 13: B I almost missed this. Please make it thicker, so that people will see it.
For example, explain what to do a backup for (so that there's a possibility to recover, and also machine-specific data like MAC addresses), how to do a backup, why one backup is not enough (at least one copy should be off-site, just in case), why using me_cleaned firmwares for coreboot porting is bad (remember p8h61-m pro and LPC not working fine without a working ME? imagine that, but while porting)...
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 16: Try to find out as much as possible about the current hardware. : Dump information with existing tools: : * lspci : * lsusb : * superiotool : * inteltool : * ectool : * dmidecode : * acpidump
Maybe: If you are porting a platform from an existing BIOS, try to find out...
Maybe "guard" this with an "if you are retrofitting coreboot to a mainboard which already has a working firmware"
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 26: Have a look at the mainboard schematics to see how those components are : connected. Sometimes you don't have schematics/boardviews (you really should, if you're not retrofitting coreboot).
I would suggest focusing on GPIOs, as this is usually the most dangerous thing coreboot configures (get the GPIO configs wrong, and you can short-circuit things)
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 33: It a good idea to find a similar board and use it as reference. Add the following remark:
Prefer a board from a different vendor which uses the same platform (chipset) over a board from the same vendor, but which uses a different platform.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 36: 3. Find a similar SuperIO or BMC : To debug coreboot you should get the serial console working first. If your : board has a SuperIO or BMC try to find a compatible one. : Some boards have UART hardware on the SoC, making external components : obsolete. Maybe rewrite as "Find a usable debug console". I would say it is more important than a SuperIO/BMC
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 41: 4. Start with a minimal bootable configuration. That is: Since some people tend to just change all the things in menuconfig, I'd add a recommendation to not do so. The less that deviates from defaults, the easier to track down issues is.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 45: 5. Be able to recover from a bad flash. You should use external flashing as : it's likely that you will brick your platform on the first attempts. : Make sure to backup the original firmware (if any). IMHO, this should be the first thing. If somebody is unable to get a recovery method, then they should reconsider things, because porting things will be impossible for them.
Some people just don't know how to recover from a bad flash (just need knowledge and the tools to do it), whereas others have imposed restrictions which makes it impossible for them to have a reliable recovery method...
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 53: scheme instead.
When it makes sense.
I would prefer to tell somebody to use the variants mechanism, than to have them undo the variants because the result is just spaghetti.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 54: logical should be an adverb: logically
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 63: What you should **not** document:
You might want to make that more clear here then: […]
I would suggest mentioning what to do: reference existing documentation as much as possible. Especially w.r.t. flashing.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 68: Recommended a flashing method as described in [Flashing tutorial]
I see now. […]
Recommended flashing methods could also be generalized into a sorted list, and then mainboard capabilities would just decide which would be the best method. Some examples (assuming one SPI flash chip):
if the flash chip is socketed, just remove it and connect it to your programmer (no in-circuit programming)
if the flash chip isn't socketed, but the mainboard has a diode on the flash chip's VCC rail (so that the rest of the board doesn't get powered on) and also has resistors between the flash chip and the SoC, then it's recommended to do in-circuit programming (board is designed for it).
if the flash chip isn't socketed, has no protection diode nor resistors, and also has a BMC/EC connected to the flash chip, then you're screwed. In-circuit programming is pretty much impossible. If you can attach a second flash chip and disable the on-board flash chip, you could use that (no in-circuit programming). Otherwise, you would have to desolder the flash chip (and if it's WSON... you're gonna have a good time)
Another weird case I have: Toshiba A300-1ME laptop (mainboard name: Inventec Potomac 10G). The board has a single SPI flash chip on the EC (board boots from LPC, using the EC as a translator). But it also has pads for another SPI flash chip, directly connected to the southbridge (no other masters around). In that case, I could have done several things. However, the best solution I found was to wire up the strap that changes the default boot location between LPC and SPI to a switch (the wifi rfkill switch), install a 2nd flash chip to put coreboot on, leave the vendor firmware on the EC-attached flash chip, and flash internally. While it might seem rather complicated, this setup means I can do all the coreboot tests I want with a closed chassis, and without *no* additional hardware to recover from a bad flash, just flipping the rfkill switch does the trick. I'd call this an absolute win over any other recovery method :-)
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 70: schematics schematics/boardviews
You don't sound like you ever had to hunt down R347 on a mainboard without a boardview... ಠ_ಠ
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 72: The picture should be less than 800px in width and compressed with : 70% compression to reduce size. How about specifying a size in kiB as a reference? It's easier to check the size while reviewing (it literally appears just there).
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 77: Once done submit your board to Gerrit. How? We have lesson2 for that, so maybe link it here?
Also, encourage people to use gerrit from the very beginning: people can point out mistakes, improvements, suggestions... And if you want your new board merged, you will have to use gerrit anyway.
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... File Documentation/getting_started/writing_documentation.md:
https://review.coreboot.org/c/coreboot/+/34632/4/Documentation/getting_start... PS4, Line 52: New mainboard ports should also add documentation
"It's requested, although not required that new mainboards add documentation. […]
"it is highly appreciated" ?