Pablo has posted comments on this change. ( https://review.coreboot.org/15057 )
Change subject: payloads/external/tianocore: Update to build uefi corebootpayload
......................................................................
Patch Set 20:
A popular package for Arch Linux builds the uefi shell on gcc5 (and basically the latest software stack) doing some interesting mending to the sources that could be useful to building the whole edk2.
Link to build script:
https://aur.archlinux.org/cgit/aur.git/plain/PKGBUILD?h=uefi-shell-git
--
To view, visit https://review.coreboot.org/15057
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9719ca5c39fccb856dfe096d449760a937d51fd1
Gerrit-PatchSet: 20
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com>
Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Maurice Ma
Gerrit-Reviewer: Maurice Ma <mauricexma(a)gmail.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com>
Gerrit-Reviewer: Pablo
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Prabal Saha <coolstarorganization(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/19090 )
Change subject: nb/intel/gm45: Allow setting backlight pwm frequency from CBFS
......................................................................
Patch Set 2:
> (5 comments)
>
> I'm not sure about this. I don't want it to be easy for a user to
> not
> participate (e.g. not telling us about his panel). Also, I don't
> believe
> that this is easier than recompiling ;) Though, maybe it is for
> some
> blobby downstream distribution.
>
> And, testing values is actually easier with a simple MMIO write
> from
> the OS.
Yes testing should be done from OS. It would be nice to document it (or have a tool since reg holds the divisor).
I agree that not having it is more likely to get participation into adding entries to the lookup table.
I guess this indeed mostly makes sense if you are tied to a certain git commit like a cb release or if one uses a binary, however it could also be a handy way of working around aging CCFL or weird working inverters.
--
To view, visit https://review.coreboot.org/19090
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e90e47ffc42b8d7062af87f6b174cd03445bb7
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kevin Keijzer <kevin(a)librepractice.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19558 )
Change subject: google/gru: support 800M/928M frequency for bob
......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/19558/6/src/mainboard/google/gru/sdram_conf…
File src/mainboard/google/gru/sdram_configs.c:
Line 39: [6] = "sdram-lpddr3-micron-4GB-928",
...and still you have two tables? The whole point of using snprintf was that you wouldn't need to have multiple tables anymore! (Use "%s-%d" to add the frequency.)
Line 53: sdram_configs = sdram_configs_928;
nit: would be better to factor out into a separate function, I think (i.e. int get_sdram_target_mhz(void) that returns either 800 or 928).
https://review.coreboot.org/#/c/19558/6/src/mainboard/google/gru/sdram_para…
File src/mainboard/google/gru/sdram_params/Makefile.inc:
Line 22: sdram-params += sdram-lpddr3-samsung-2GB-24EB-928
nit: would look better grouping all frequencies of the same chip together, with a blank line between chips, I think?
--
To view, visit https://review.coreboot.org/19558
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I613050292a09ff56f4636d7af285075e32259ef4
Gerrit-PatchSet: 6
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/19090 )
Change subject: nb/intel/gm45: Allow setting backlight pwm frequency from CBFS
......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/19090/2/src/northbridge/intel/gm45/gma.c
File src/northbridge/intel/gm45/gma.c:
Line 668: printk(BIOS_ERR, "gma: Error reading blc pwm from CBFS\n");
> line too long
I'll adapt the codeflow so that there is less indentation.
Line 678: #define MINIMUM_BLC_PWM 50
> seems very low
It is... maybe 100?
PS2, Line 745: (blc_pwm_cbfs >= MINIMUM_BLC_PWM) && (blc_pwm_cbfs < 0xffff)
> Better check this before casting to `int`.
Since it is actually cast into an int just checking that the value is above the (positive) minimal should do it right, or is that shady (mis)use of casts?
--
To view, visit https://review.coreboot.org/19090
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e90e47ffc42b8d7062af87f6b174cd03445bb7
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kevin Keijzer <kevin(a)librepractice.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19622 )
Change subject: src/include: remove the __ROMCC__ to enable snprintf
......................................................................
Patch Set 3:
edit: Oh, and only now I saw your comment. But the code you referred to is no longer relevant? This used to say __PRE_RAM__ but it was changed to __ROMCC__ in https://review.coreboot.org/17289, which I presume was correct (since Aaron put it there and he would probably know best).
--
To view, visit https://review.coreboot.org/19622
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6966dc8ebc911b954bc5ea8981df093df226dd6c
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19622 )
Change subject: src/include: remove the __ROMCC__ to enable snprintf
......................................................................
Patch Set 3:
I don't understand why you need this? __ROMCC__ should not be defined for ARM boards at all. I mean, I don't know if this patch makes sense or not on its own (I don't know much about ROMCC), but I am pretty certain that it has nothing to do with what you really want to do.
Looks like vsprintf was already added to all stages recently (https://review.coreboot.org/19366), so you won't need to do anything extra there. Sorry for misleading you.
--
To view, visit https://review.coreboot.org/19622
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6966dc8ebc911b954bc5ea8981df093df226dd6c
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No