coreboot-gerrit January 2019

coreboot-gerrit@coreboot.org

1 participants
1296 discussions

Change in ...coreboot[master]: string.h: allow to compile strdup in romstage
by Thomas Heijligen (Code Review) 30 Jan '19

30 Jan '19

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30844 Change subject: string.h: allow to compile strdup in romstage ...................................................................... string.h: allow to compile strdup in romstage Allows to use ramstage compilation units in romstage as long as strdup isn't called in romstage. Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M src/include/string.h 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/30844/1 diff --git a/src/include/string.h b/src/include/string.h index fc96393..b692de9 100644 --- a/src/include/string.h +++ b/src/include/string.h @@ -51,7 +51,7 @@ return 0; } -#if !defined(__PRE_RAM__) +//#if !defined(__PRE_RAM__) static inline char *strdup(const char *s) { size_t sz = strlen(s) + 1; @@ -69,7 +69,7 @@ memcpy(d + sz_1, s2, sz_2 + 1); return d; } -#endif +//#endif /** * Find a character in a string. -- To view, visit https://review.coreboot.org/c/coreboot/+/30844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed Gerrit-Change-Number: 30844 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

6 19

Change in ...coreboot[master]: Documentation: Add coding style
by Arthur Heymans (Code Review) 30 Jan '19

30 Jan '19

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30834 Change subject: Documentation: Add coding style ...................................................................... Documentation: Add coding style This is the old wiki page https://www.coreboot.org/Coding_Style coverted from mediawiki to markdown. Change-Id: Id56a8b7500121c4d9c18bc0b6bbc2c05402268dc Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- A Documentation/coding_style.md M Documentation/index.md 2 files changed, 939 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/30834/1 diff --git a/Documentation/coding_style.md b/Documentation/coding_style.md new file mode 100644 index 0000000..ae957c2 --- /dev/null +++ b/Documentation/coding_style.md @@ -0,0 +1,938 @@ +# Coding Style + +This is a short document describing the preferred coding style for the +coreboot project. It is in many ways exactly the same as the Linux +kernel coding style. In fact, most of this document has been copied from +the [Linux kernel coding style](http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain… + +Please at least consider the points made here. + +First off, I'd suggest printing out a copy of the GNU coding standards, +and NOT read it. Burn them, it's a great symbolic gesture. + +Anyway, here goes: + +## Indentation + +Tabs are 8 characters, and thus indentations are also 8 characters. +There are heretic movements that try to make indentations 4 (or even 2!) +characters deep, and that is akin to trying to define the value of PI to +be 3. + +Rationale: The whole idea behind indentation is to clearly define where +a block of control starts and ends. Especially when you've been looking +at your screen for 20 straight hours, you'll find it a lot easier to +see how the indentation works if you have large indentations. + +Now, some people will claim that having 8-character indentations makes +the code move too far to the right, and makes it hard to read on a +80-character terminal screen. The answer to that is that if you need +more than 3 levels of indentation, you're screwed anyway, and should +fix your program. + +In short, 8-char indents make things easier to read, and have the added +benefit of warning you when you're nesting your functions too deep. +Heed that warning. + +The preferred way to ease multiple indentation levels in a switch +statement is to align the "switch" and its subordinate "case" labels +in the same column instead of "double-indenting" the "case" labels. +E.g.: + +```c +switch (suffix) { +case 'G': +case 'g': + mem <<= 30; + break; +case 'M': +case 'm': + mem <<= 20; + break; +case 'K': +case 'k': + mem <<= 10; + /* fall through */ +default: + break; +} +``` + +Don't put multiple statements on a single line unless you have +something to hide: + +```c +if (condition) do_this; + do_something_everytime; +``` + +Don't put multiple assignments on a single line either. Kernel coding +style is super simple. Avoid tricky expressions. + +Outside of comments, documentation and except in Kconfig, spaces are +never used for indentation, and the above example is deliberately +broken. + +Get a decent editor and don't leave whitespace at the end of lines. + +## Breaking long lines and strings + +Coding style is all about readability and maintainability using commonly +available tools. + +The limit on the length of lines is 80 columns and this is a strongly +preferred limit. + +Statements longer than 80 columns will be broken into sensible chunks, +unless exceeding 80 columns significantly increases readability and does +not hide information. Descendants are always substantially shorter than +the parent and are placed substantially to the right. The same applies +to function headers with a long argument list. However, never break +user-visible strings such as printk messages, because that breaks the +ability to grep for them. + +## Placing Braces and Spaces + +The other issue that always comes up in C styling is the placement of +braces. Unlike the indent size, there are few technical reasons to +choose one placement strategy over the other, but the preferred way, as +shown to us by the prophets Kernighan and Ritchie, is to put the opening +brace last on the line, and put the closing brace first, thusly: + +```c +if (x is true) { + we do y +} +``` + +This applies to all non-function statement blocks (if, switch, for, +while, do). E.g.: + +```c +switch (action) { +case KOBJ_ADD: + return "add"; +case KOBJ_REMOVE: + return "remove"; +case KOBJ_CHANGE: + return "change"; +default: + return NULL; +} +``` + +However, there is one special case, namely functions: they have the +opening brace at the beginning of the next line, thus: + +```c +int function(int x) +{ + body of function +} +``` + +Heretic people all over the world have claimed that this inconsistency +is ... well ... inconsistent, but all right-thinking people know that +(a) K&R are _right_ and (b) K&R are right. Besides, functions are +special anyway (you can't nest them in C). + +Note that the closing brace is empty on a line of its own, _except_ in +the cases where it is followed by a continuation of the same statement, +ie a "while" in a do-statement or an "else" in an if-statement, like +this: + +```c +do { + body of do-loop +} while (condition); +``` + +and + +```c +if (x == y) { + .. +} else if (x > y) { + ... +} else { + .... +} +``` + +Rationale: K&R. + +Also, note that this brace-placement also minimizes the number of empty +(or almost empty) lines, without any loss of readability. Thus, as the +supply of new-lines on your screen is not a renewable resource (think +25-line terminal screens here), you have more empty lines to put +comments on. + +Do not unnecessarily use braces where a single statement will do. + +```c +if (condition) + action(); +``` + +and + +```c +if (condition) + do_this(); +else + do_that(); +``` + +This does not apply if only one branch of a conditional statement is a +single statement; in the latter case use braces in both branches: + +```c +if (condition) { + do_this(); + do_that(); +} else { + otherwise(); +} +``` + +### Spaces + +Linux kernel style for use of spaces depends (mostly) on +function-versus-keyword usage. Use a space after (most) keywords. The +notable exceptions are sizeof, typeof, alignof, and __attribute__, +which look somewhat like functions (and are usually used with +parentheses in Linux, although they are not required in the language, as +in: "sizeof info" after "struct fileinfo info;" is declared). + +So use a space after these keywords: + +``` +if, switch, case, for, do, while +``` + +but not with sizeof, typeof, alignof, or __attribute__. E.g., + +```c +s = sizeof(struct file); +``` + +Do not add spaces around (inside) parenthesized expressions. This +example is + +- bad*: + +```c +s = sizeof( struct file ); +``` + +When declaring pointer data or a function that returns a pointer type, +the preferred use of '*' is adjacent to the data name or function +name and not adjacent to the type name. Examples: + +```c +char *linux_banner; +unsigned long long memparse(char *ptr, char **retptr); +char *match_strdup(substring_t *s); +``` + +Use one space around (on each side of) most binary and ternary +operators, such as any of these: + +``` += + - < > * / % | & ^ <= >= == != ? : +``` + +but no space after unary operators: + +``` +& * + - ~ ! sizeof typeof alignof __attribute__ defined +``` + +no space before the postfix increment & decrement unary operators: + +``` +++ -- +``` + +no space after the prefix increment & decrement unary operators: + +``` +++ -- +``` + +and no space around the '.' and "->" structure member operators. + +Do not leave trailing whitespace at the ends of lines. Some editors with +"smart" indentation will insert whitespace at the beginning of new +lines as appropriate, so you can start typing the next line of code +right away. However, some such editors do not remove the whitespace if +you end up not putting a line of code there, such as if you leave a +blank line. As a result, you end up with lines containing trailing +whitespace. + +Git will warn you about patches that introduce trailing whitespace, and +can optionally strip the trailing whitespace for you; however, if +applying a series of patches, this may make later patches in the series +fail by changing their context lines. + +### Naming + +C is a Spartan language, and so should your naming be. Unlike Modula-2 +and Pascal programmers, C programmers do not use cute names like +ThisVariableIsATemporaryCounter. A C programmer would call that variable +"tmp", which is much easier to write, and not the least more difficult +to understand. + +HOWEVER, while mixed-case names are frowned upon, descriptive names for +global variables are a must. To call a global function "foo" is a +shooting offense. + +GLOBAL variables (to be used only if you _really_ need them) need to +have descriptive names, as do global functions. If you have a function +that counts the number of active users, you should call that +"count_active_users()" or similar, you should _not_ call it +"cntusr()". + +Encoding the type of a function into the name (so-called Hungarian +notation) is brain damaged - the compiler knows the types anyway and can +check those, and it only confuses the programmer. No wonder MicroSoft +makes buggy programs. + +LOCAL variable names should be short, and to the point. If you have some +random integer loop counter, it should probably be called "i". Calling +it "loop_counter" is non-productive, if there is no chance of it +being mis-understood. Similarly, "tmp" can be just about any type of +variable that is used to hold a temporary value. + +If you are afraid to mix up your local variable names, you have another +problem, which is called the function-growth-hormone-imbalance syndrome. +See chapter 6 (Functions). + +## Typedefs + +Please don't use things like "vps_t". + +It's a _mistake_ to use typedef for structures and pointers. When you +see a + +```c +vps_t a; +``` + +in the source, what does it mean? + +In contrast, if it says + +```c +struct virtual_container *a; +``` + +you can actually tell what "a" is. + +Lots of people think that typedefs "help readability". Not so. They +are useful only for: + +(a) totally opaque objects (where the typedef is actively used to +_hide_ what the object is). + +Example: "pte_t" etc. opaque objects that you can only access using +the proper accessor functions. + +NOTE! Opaqueness and "accessor functions" are not good in themselves. +The reason we have them for things like pte_t etc. is that there really +is absolutely _zero_ portably accessible information there. + +(b) Clear integer types, where the abstraction _helps_ avoid confusion +whether it is "int" or "long". + +u8/u16/u32 are perfectly fine typedefs, although they fit into category +(d) better than here. + +NOTE! Again - there needs to be a _reason_ for this. If something is +"unsigned long", then there's no reason to do + +```c +typedef unsigned long myflags_t; +``` + +but if there is a clear reason for why it under certain circumstances +might be an "unsigned int" and under other configurations might be +"unsigned long", then by all means go ahead and use a typedef. + +(c) when you use sparse to literally create a _new_ type for +type-checking. + +(d) New types which are identical to standard C99 types, in certain +exceptional circumstances. + +Although it would only take a short amount of time for the eyes and +brain to become accustomed to the standard types like 'uint32_t', +some people object to their use anyway. + +Therefore, the Linux-specific 'u8/u16/u32/u64' types and their signed +equivalents which are identical to standard types are permitted -- +although they are not mandatory in new code of your own. + +When editing existing code which already uses one or the other set of +types, you should conform to the existing choices in that code. + +(e) Types safe for use in userspace. + +In certain structures which are visible to userspace, we cannot require +C99 types and cannot use the 'u32' form above. Thus, we use __u32 +and similar types in all structures which are shared with userspace. + +Maybe there are other cases too, but the rule should basically be to +NEVER EVER use a typedef unless you can clearly match one of those +rules. + +In general, a pointer, or a struct that has elements that can reasonably +be directly accessed should _never_ be a typedef. + +## Functions + +Functions should be short and sweet, and do just one thing. They should +fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, +as we all know), and do one thing and do that well. + +The maximum length of a function is inversely proportional to the +complexity and indentation level of that function. So, if you have a +conceptually simple function that is just one long (but simple) +case-statement, where you have to do lots of small things for a lot of +different cases, it's OK to have a longer function. + +However, if you have a complex function, and you suspect that a +less-than-gifted first-year high-school student might not even +understand what the function is all about, you should adhere to the +maximum limits all the more closely. Use helper functions with +descriptive names (you can ask the compiler to in-line them if you think +it's performance-critical, and it will probably do a better job of it +than you would have done). + +Another measure of the function is the number of local variables. They +shouldn't exceed 5-10, or you're doing something wrong. Re-think the +function, and split it into smaller pieces. A human brain can generally +easily keep track of about 7 different things, anything more and it gets +confused. You know you're brilliant, but maybe you'd like to +understand what you did 2 weeks from now. + +In source files, separate functions with one blank line. If the function +is exported, the EXPORT* macro for it should follow immediately after +the closing function brace line. E.g.: + +```c +int system_is_up(void) +{ + return system_state == SYSTEM_RUNNING; +} +EXPORT_SYMBOL(system_is_up); +``` + +In function prototypes, include parameter names with their data types. +Although this is not required by the C language, it is preferred in +Linux because it is a simple way to add valuable information for the +reader. + +## Centralized exiting of functions + +Albeit deprecated by some people, the equivalent of the goto statement +is used frequently by compilers in form of the unconditional jump +instruction. + +The goto statement comes in handy when a function exits from multiple +locations and some common work such as cleanup has to be done. If there +is no cleanup needed then just return directly. + +The rationale is: + +- unconditional statements are easier to understand and follow +- nesting is reduced +- errors by not updating individual exit points when making + modifications are prevented +- saves the compiler work to optimize redundant code away ;) + +```c +int fun(int a) +{ + int result = 0; + char *buffer = kmalloc(SIZE); + + if (buffer == NULL) + return -ENOMEM; + + if (condition1) { + while (loop1) { + ... + } + result = 1; + goto out; + } + ... + out: + kfree(buffer); + return result; +} +``` + +## Commenting + +Comments are good, but there is also a danger of over-commenting. NEVER +try to explain HOW your code works in a comment: it's much better to +write the code so that the _working_ is obvious, and it's a waste of +time to explain badly written code. + +Generally, you want your comments to tell WHAT your code does, not HOW. +Also, try to avoid putting comments inside a function body: if the +function is so complex that you need to separately comment parts of it, +you should probably go back to chapter 6 for a while. You can make small +comments to note or warn about something particularly clever (or ugly), +but try to avoid excess. Instead, put the comments at the head of the +function, telling people what it does, and possibly WHY it does it. + +When commenting the kernel API functions, please use the kernel-doc +format. See the files Documentation/kernel-doc-nano-HOWTO.txt and +scripts/kernel-doc for details. + +coreboot style for comments is the C89 "/* ... */" style. You may +use C99-style "// ..." comments. + +The preferred style for *short* (multi-line) comments is: + +```c +/* This is the preferred style for short multi-line + comments in the Linux kernel source code. + Please use it consistently. */ +``` + +The preferred style for *long* (multi-line) comments is: + +```c +/* + * This is the preferred style for multi-line + * comments in the Linux kernel source code. + * Please use it consistently. + * + * Description: A column of asterisks on the left side, + * with beginning and ending almost-blank lines. + */ +``` + +It's also important to comment data, whether they are basic types or +derived types. To this end, use just one data declaration per line (no +commas for multiple data declarations). This leaves you room for a small +comment on each item, explaining its use. + +## You've made a mess of it +That's OK, we all do. You've probably been told by your long-time Unix user +helper that "GNU emacs" automatically formats the C sources for you, and +you've noticed that yes, it does do that, but the defaults it uses are less +than desirable (in fact, they are worse than random typing - an infinite +number of monkeys typing into GNU emacs would never make a good program). + +So, you can either get rid of GNU emacs, or change it to use saner values. +To do the latter, you can stick the following in your .emacs file: + +```lisp +(defun c-lineup-arglist-tabs-only (ignored) + "Line up argument lists by tabs, not spaces" + (let* ((anchor (c-langelem-pos c-syntactic-element)) + (column (c-langelem-2nd-pos c-syntactic-element)) + (offset (- (1+ column) anchor)) + (steps (floor offset c-basic-offset))) + (* (max steps 1) + c-basic-offset))) + +(add-hook 'c-mode-common-hook + (lambda () + ;; Add kernel style + (c-add-style + "linux-tabs-only" + '("linux" (c-offsets-alist + (arglist-cont-nonempty + c-lineup-gcc-asm-reg + c-lineup-arglist-tabs-only)))))) + +(add-hook 'c-mode-hook + (lambda () + (let ((filename (buffer-file-name))) + ;; Enable kernel mode for the appropriate files + (when (and filename + (string-match (expand-file-name "~/src/linux-trees") + filename)) + (setq indent-tabs-mode t) + (c-set-style "linux-tabs-only"))))) +``` + +This will make emacs go better with the kernel coding style for C files +below ~/src/linux-trees. + +But even if you fail in getting emacs to do sane formatting, not +everything is lost: use "indent". + +Now, again, GNU indent has the same brain-dead settings that GNU emacs +has, which is why you need to give it a few command line options. +However, that's not too bad, because even the makers of GNU indent +recognize the authority of K&R (the GNU people aren't evil, they are +just severely misguided in this matter), so you just give indent the +options "-kr -i8" (stands for "K&R, 8 character indents"), or use +"scripts/Lindent", which indents in the latest style. + +"indent" has a lot of options, and especially when it comes to comment +re-formatting you may want to take a look at the man page. But remember: +"indent" is not a fix for bad programming. + +## Kconfig configuration files + +For all of the Kconfig* configuration files throughout the source tree, +the indentation is somewhat different. Lines under a "config" +definition are indented with one tab, while help text is indented an +additional two spaces. Example: + +```kconfig +config AUDIT + bool "Auditing support" + depends on NET + help + Enable auditing infrastructure that can be used with another + kernel subsystem, such as SELinux (which requires this for + logging of avc messages output). Does not do system-call + auditing without CONFIG_AUDITSYSCALL. +``` + +Seriously dangerous features (such as write support for certain +filesystems) should advertise this prominently in their prompt string: + +```kconfig +config ADFS_FS_RW + bool "ADFS write support (DANGEROUS)" + depends on ADFS_FS + ... +``` + +For full documentation on the configuration files, see the file +Documentation/kbuild/kconfig-language.txt. + +Data structures +--------------- + +Data structures that have visibility outside the single-threaded +environment they are created and destroyed in should always have +reference counts. In the kernel, garbage collection doesn't exist (and +outside the kernel garbage collection is slow and inefficient), which +means that you absolutely _have_ to reference count all your uses. + +Reference counting means that you can avoid locking, and allows multiple +users to have access to the data structure in parallel - and not having +to worry about the structure suddenly going away from under them just +because they slept or did something else for a while. + +Note that locking is _not_ a replacement for reference counting. +Locking is used to keep data structures coherent, while reference +counting is a memory management technique. Usually both are needed, and +they are not to be confused with each other. + +Many data structures can indeed have two levels of reference counting, +when there are users of different "classes". The subclass count counts +the number of subclass users, and decrements the global count just once +when the subclass count goes to zero. + +Examples of this kind of "multi-level-reference-counting" can be found +in memory management ("struct mm_struct": mm_users and mm_count), +and in filesystem code ("struct super_block": s_count and +s_active). + +Remember: if another thread can find your data structure, and you don't +have a reference count on it, you almost certainly have a bug. + +Macros, Enums and RTL +--------------------- + +Names of macros defining constants and labels in enums are capitalized. + +```c +#define CONSTANT 0x12345 +``` + +Enums are preferred when defining several related constants. + +CAPITALIZED macro names are appreciated but macros resembling functions +may be named in lower case. + +Generally, inline functions are preferable to macros resembling +functions. + +Macros with multiple statements should be enclosed in a do - while +block: + +```c +#define macrofun(a, b, c) \ + do { \ + if (a == 5) \ + do_this(b, c); \ + } while (0) +``` + +Things to avoid when using macros: + +1) macros that affect control flow: + +```c +#define FOO(x) \ + do { \ + if (blah(x) < 0) \ + return -EBUGGERED; \ + } while(0) +``` + +is a *very* bad idea. It looks like a function call but exits the +"calling" function; don't break the internal parsers of those who +will read the code. + +2) macros that depend on having a local variable with a magic name: + +```c +#define FOO(val) bar(index, val) +``` + +might look like a good thing, but it's confusing as hell when one reads +the code and it's prone to breakage from seemingly innocent changes. + +3) macros with arguments that are used as l-values: FOO(x) = y; will +bite you if somebody e.g. turns FOO into an inline function. + +4) forgetting about precedence: macros defining constants using +expressions must enclose the expression in parentheses. Beware of +similar issues with macros using parameters. + +```c +#define CONSTANT 0x4000 +#define CONSTEXP (CONSTANT | 3) +``` + +The cpp manual deals with macros exhaustively. The gcc internals manual +also covers RTL which is used frequently with assembly language in the +kernel. + +Printing kernel messages +------------------------ + +Kernel developers like to be seen as literate. Do mind the spelling of +kernel messages to make a good impression. Do not use crippled words +like "dont"; use "do not" or "don't" instead. Make the messages +concise, clear, and unambiguous. + +Kernel messages do not have to be terminated with a period. + +Printing numbers in parentheses (%d) adds no value and should be +avoided. + +There are a number of driver model diagnostic macros in +<linux/device.h> which you should use to make sure messages are +matched to the right device and driver, and are tagged with the right +level: dev_err(), dev_warn(), dev_info(), and so forth. For messages +that aren't associated with a particular device, <linux/printk.h> +defines pr_debug() and pr_info(). + +Coming up with good debugging messages can be quite a challenge; and +once you have them, they can be a huge help for remote troubleshooting. +Such messages should be compiled out when the DEBUG symbol is not +defined (that is, by default they are not included). When you use +dev_dbg() or pr_debug(), that's automatic. Many subsystems have +Kconfig options to turn on -DDEBUG. A related convention uses +VERBOSE_DEBUG to add dev_vdbg() messages to the ones already enabled +by DEBUG. + +Allocating memory +----------------- + +Coreboot provides a single general purpose memory allocator: malloc() + +The preferred form for passing a size of a struct is the following: + +```c +p = malloc(sizeof(*p)); +``` + +The alternative form where struct name is spelled out hurts readability +and introduces an opportunity for a bug when the pointer variable type +is changed but the corresponding sizeof that is passed to a memory +allocator is not. + +Casting the return value which is a void pointer is redundant. The +conversion from void pointer to any other pointer type is guaranteed by +the C programming language. + +You should contain your memory usage to stack variables whenever +possible. Only use malloc() as a last resort. In ramstage, you may also +be able to get away with using static variables. Never use malloc() +outside of ramstage. + +Since coreboot only runs for a very short time, there is no memory +deallocator, although a corresponding free() is offered. It is a no-op. +Use of free() is not required though it is accepted. It is useful when +sharing code with other codebases that make use of free(). + +The inline disease +------------------ + +There appears to be a common misperception that gcc has a magic "make +me faster" speedup option called "inline". While the use of inlines +can be appropriate (for example as a means of replacing macros, see +Chapter 12), it very often is not. Abundant use of the inline keyword +leads to a much bigger kernel, which in turn slows the system as a whole +down, due to a bigger icache footprint for the CPU and simply because +there is less memory available for the pagecache. Just think about it; a +pagecache miss causes a disk seek, which easily takes 5 milliseconds. +There are a LOT of cpu cycles that can go into these 5 milliseconds. + +A reasonable rule of thumb is to not put inline at functions that have +more than 3 lines of code in them. An exception to this rule are the +cases where a parameter is known to be a compiletime constant, and as a +result of this constantness you *know* the compiler will be able to +optimize most of your function away at compile time. For a good example +of this later case, see the kmalloc() inline function. + +Often people argue that adding inline to functions that are static and +used only once is always a win since there is no space tradeoff. While +this is technically correct, gcc is capable of inlining these +automatically without help, and the maintenance issue of removing the +inline when a second user appears outweighs the potential value of the +hint that tells gcc to do something it would have done anyway. + +Function return values and names +-------------------------------- + +Functions can return values of many different kinds, and one of the most +common is a value indicating whether the function succeeded or failed. +Such a value can be represented as an error-code integer (-Exxx = +failure, 0 = success) or a "succeeded" boolean (0 = failure, non-zero += success). + +Mixing up these two sorts of representations is a fertile source of +difficult-to-find bugs. If the C language included a strong distinction +between integers and booleans then the compiler would find these +mistakes for us... but it doesn't. To help prevent such bugs, always +follow this convention: + +If the name of a function is an action or an imperative command, +the function should return an error-code integer. If the name +is a predicate, the function should return a "succeeded" boolean. + +For example, "add work" is a command, and the add_work() function +returns 0 for success or -EBUSY for failure. In the same way, "PCI +device present" is a predicate, and the pci_dev_present() function +returns 1 if it succeeds in finding a matching device or 0 if it +doesn't. + +All EXPORTed functions must respect this convention, and so should all +public functions. Private (static) functions need not, but it is +recommended that they do. + +Functions whose return value is the actual result of a computation, +rather than an indication of whether the computation succeeded, are not +subject to this rule. Generally they indicate failure by returning some +out-of-range result. Typical examples would be functions that return +pointers; they use NULL or the ERR_PTR mechanism to report failure. + +Don't re-invent the kernel macros +---------------------------------- + +The header file include/linux/kernel.h contains a number of macros that +you should use, rather than explicitly coding some variant of them +yourself. For example, if you need to calculate the length of an array, +take advantage of the macro + +```c +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) +``` + +There are also min() and max() macros that do strict type checking if +you need them. Feel free to peruse that header file to see what else is +already defined that you shouldn't reproduce in your code. + +Editor modelines and other cruft +-------------------------------- + +Some editors can interpret configuration information embedded in source +files, indicated with special markers. For example, emacs interprets +lines marked like this: + +``` +-*- mode: c -*- +``` + +Or like this: + +``` +/* +Local Variables: +compile-command: "gcc -DMAGIC_DEBUG_FLAG foo.c" +End: +*/ +``` + +Vim interprets markers that look like this: + +``` +/* vim:set sw=8 noet */ +``` + +Do not include any of these in source files. People have their own +personal editor configurations, and your source files should not +override them. This includes markers for indentation and mode +configuration. People may use their own custom mode, or may have some +other magic method for making indentation work correctly. + +Inline assembly +--------------- + +In architecture-specific code, you may need to use inline assembly to +interface with CPU or platform functionality. Don't hesitate to do so +when necessary. However, don't use inline assembly gratuitously when C +can do the job. You can and should poke hardware from C when possible. + +Consider writing simple helper functions that wrap common bits of inline +assembly, rather than repeatedly writing them with slight variations. +Remember that inline assembly can use C parameters. + +Large, non-trivial assembly functions should go in .S files, with +corresponding C prototypes defined in C header files. The C prototypes +for assembly functions should use "asmlinkage". + +You may need to mark your asm statement as volatile, to prevent GCC from +removing it if GCC doesn't notice any side effects. You don't always +need to do so, though, and doing so unnecessarily can limit +optimization. + +When writing a single inline assembly statement containing multiple +instructions, put each instruction on a separate line in a separate +quoted string, and end each string except the last with nt to +properly indent the next instruction in the assembly output: + +```c +asm ("magic %reg1, #42nt" + "more_magic %reg2, %reg3" + : /* outputs */ : /* inputs */ : /* clobbers */); +``` + +References +---------- + +The C Programming Language, Second Edition by Brian W. Kernighan and +Dennis M. Ritchie. Prentice Hall, Inc., 1988. ISBN 0-13-110362-8 +(paperback), 0-13-110370-9 (hardback). URL: +<http://cm.bell-labs.com/cm/cs/cbook/> + +The Practice of Programming by Brian W. Kernighan and Rob Pike. +Addison-Wesley, Inc., 1999. ISBN 0-201-61586-X. URL: +<http://cm.bell-labs.com/cm/cs/tpop/> + +GNU manuals - where in compliance with K&R and this text - for cpp, gcc, +gcc internals and indent, all available from +<http://www.gnu.org/manual/> + +WG14 is the international standardization working group for the +programming language C, URL: <http://www.open-std.org/JTC1/SC22/WG14/> + +Kernel CodingStyle, by greg(a)kroah.com at OLS 2002: +<http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/> diff --git a/Documentation/index.md b/Documentation/index.md index 9fdb0dc..5c38909 100644 --- a/Documentation/index.md +++ b/Documentation/index.md @@ -9,6 +9,7 @@ * [Getting Started](getting_started/index.md) * [Rookie Guide](lessons/index.md) +* [Coding Style](coding_style.md) * [Code of Conduct](code_of_conduct.md) * [Timestamps](timestamp.md) * [Intel IFD Binary Extraction](Binary_Extraction.md) -- To view, visit https://review.coreboot.org/c/coreboot/+/30834 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id56a8b7500121c4d9c18bc0b6bbc2c05402268dc Gerrit-Change-Number: 30834 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

5 7

Change in ...coreboot[master]: mainboard/intel/cannonlake_rvp: Enable SaGv config
by Patrick Georgi (Code Review) 30 Jan '19

30 Jan '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31132 ) Change subject: mainboard/intel/cannonlake_rvp: Enable SaGv config ...................................................................... mainboard/intel/cannonlake_rvp: Enable SaGv config This patch enables SaGv on Intel CNL-Y and CNL-U RVP board Change-Id: I8a4b8a2a365caed304935bf0d66db9a92d10c23f Signed-off-by: Ronak Kanabar <ronak.kanabar(a)intel.com> Reviewed-on: https://review.coreboot.org/c/31132 Reviewed-by: Furquan Shaikh <furquan(a)google.com> Reviewed-by: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb M src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb 2 files changed, 2 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Pratikkumar V Prajapati: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb b/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb index 9604210..e5f867c 100644 --- a/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb +++ b/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "SaGv_FixedHigh" + register "SaGv" = "SaGv_Enabled" register "ScsEmmcHs400Enabled" = "1" register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" diff --git a/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb b/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb index e2ebaba..a6d329b 100644 --- a/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb +++ b/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "SaGv_FixedHigh" + register "SaGv" = "SaGv_Enabled" register "ScsEmmcHs400Enabled" = "1" register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" -- To view, visit https://review.coreboot.org/c/coreboot/+/31132 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8a4b8a2a365caed304935bf0d66db9a92d10c23f Gerrit-Change-Number: 31132 Gerrit-PatchSet: 3 Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in ...coreboot[master]: string: move strdup() & strconcat() to lib/string.c
by Thomas Heijligen (Code Review) 30 Jan '19

30 Jan '19

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31141 Change subject: string: move strdup() & strconcat() to lib/string.c ...................................................................... string: move strdup() & strconcat() to lib/string.c Move functions not available in PRE_RAM into seperate file. Makes it easier to share code between rom and ramstage. Change-Id: I0b9833fbf6742d110ee4bfc00cd650f219aebb2c Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M src/include/string.h M src/lib/Makefile.inc A src/lib/string.c 3 files changed, 25 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/31141/1 diff --git a/src/include/string.h b/src/include/string.h index 2f6b5f1..4a2f5e9 100644 --- a/src/include/string.h +++ b/src/include/string.h @@ -23,6 +23,8 @@ int snprintf(char *buf, size_t size, const char *fmt, ...); int vsnprintf(char *buf, size_t size, const char *fmt, va_list args); #endif +char *strdup(const char *s); +char *strconcat(const char *s1, const char *s2); // simple string functions @@ -51,26 +53,6 @@ return 0; } -#if !defined(__PRE_RAM__) -static inline char *strdup(const char *s) -{ - size_t sz = strlen(s) + 1; - char *d = malloc(sz); - memcpy(d, s, sz); - return d; -} - -static inline char *strconcat(const char *s1, const char *s2) -{ - size_t sz_1 = strlen(s1); - size_t sz_2 = strlen(s2); - char *d = malloc(sz_1 + sz_2 + 1); - memcpy(d, s1, sz_1); - memcpy(d + sz_1, s2, sz_2 + 1); - return d; -} -#endif - /** * Find a character in a string. * diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 2fd4e4c..68431f9 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -118,6 +118,7 @@ ramstage-y += fmap.c ramstage-y += memchr.c ramstage-y += memcmp.c +ramstage-y += string.c ramstage-y += malloc.c smm-$(CONFIG_SMM_TSEG) += malloc.c ramstage-y += dimm_info_util.c diff --git a/src/lib/string.c b/src/lib/string.c new file mode 100644 index 0000000..178f1fd --- /dev/null +++ b/src/lib/string.c @@ -0,0 +1,22 @@ +#include <string.h> +#include <stddef.h> +#include <stdlib.h> + +char *strdup(const char *s) +{ + size_t sz = strlen(s) + 1; + char *d = malloc(sz); + memcpy(d, s, sz); + return d; +} + +char *strconcat(const char *s1, const char *s2) +{ + size_t sz_1 = strlen(s1); + size_t sz_2 = strlen(s2); + char *d = malloc(sz_1 + sz_2 + 1); + memcpy(d, s1, sz_1); + memcpy(d + sz_1, s2, sz_2 + 1); + return d; +} + -- To view, visit https://review.coreboot.org/c/coreboot/+/31141 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0b9833fbf6742d110ee4bfc00cd650f219aebb2c Gerrit-Change-Number: 31141 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

4 6

Change in ...coreboot[master]: WIP: selfload: check target memory type in selfload
by Ting Shen (Code Review) 30 Jan '19

30 Jan '19

Ting Shen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31122 Change subject: WIP: selfload: check target memory type in selfload ...................................................................... WIP: selfload: check target memory type in selfload Change-Id: Iecdeedd9e8da67f73ac47d2a82e85b306469a626 Signed-off-by: Ting Shen <phoenixshen(a)google.com> --- M src/arch/arm64/arm_tf.c M src/include/bootmem.h M src/include/program_loading.h M src/lib/bootmem.c M src/lib/prog_loaders.c M src/lib/selfboot.c 6 files changed, 29 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31122/1 diff --git a/src/arch/arm64/arm_tf.c b/src/arch/arm64/arm_tf.c index eaf8739..b74f7be 100644 --- a/src/arch/arm64/arm_tf.c +++ b/src/arch/arm64/arm_tf.c @@ -19,6 +19,7 @@ #include <arch/transition.h> #include <arm_tf.h> #include <assert.h> +#include <bootmem.h> #include <cbfs.h> #include <program_loading.h> @@ -49,7 +50,7 @@ if (prog_locate(&bl31)) die("BL31 not found"); - if (!selfload(&bl31)) + if (!selfload_check(&bl31, BM_MEM_RESERVED)) die("BL31 load failed"); bl31_entry = prog_entry(&bl31); diff --git a/src/include/bootmem.h b/src/include/bootmem.h index 0a960c9..bfb7074 100644 --- a/src/include/bootmem.h +++ b/src/include/bootmem.h @@ -42,6 +42,7 @@ BM_MEM_OS_CUTOFF = BM_MEM_TABLE, BM_MEM_RAMSTAGE, BM_MEM_PAYLOAD, + BM_MEM_BL31, /* Reserved for BL31. */ BM_MEM_LAST, /* Last entry in this list */ }; @@ -95,7 +96,8 @@ bool bootmem_walk(range_action_t action, void *arg); /* Return 1 if region targets usable RAM, 0 otherwise. */ -int bootmem_region_targets_usable_ram(uint64_t start, uint64_t size); +int bootmem_region_targets_usable_ram(uint64_t start, uint64_t size, + enum bootmem_type dest_type); /* Allocate a temporary buffer from the unused RAM areas. */ void *bootmem_allocate_buffer(size_t size); diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 468f0b3..7be59e2 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -16,6 +16,7 @@ #ifndef PROGRAM_LOADING_H #define PROGRAM_LOADING_H +#include <bootmem.h> #include <commonlib/region.h> #include <stdint.h> #include <stddef.h> @@ -206,7 +207,7 @@ * * Defined in src/lib/selfboot.c */ -bool selfload_check(struct prog *payload); +bool selfload_check(struct prog *payload, enum bootmem_type dest_type); bool selfload(struct prog *payload); #endif /* PROGRAM_LOADING_H */ diff --git a/src/lib/bootmem.c b/src/lib/bootmem.c index 2f9b5dc..5a0f66f 100644 --- a/src/lib/bootmem.c +++ b/src/lib/bootmem.c @@ -202,7 +202,8 @@ } static int bootmem_region_targets_ram(uint64_t start, uint64_t end, - struct memranges *bm) + struct memranges *bm, + enum bootmem_type dest_type) { const struct range_entry *r; @@ -212,7 +213,7 @@ break; if (start >= range_entry_base(r) && end <= range_entry_end(r)) { - if (range_entry_tag(r) == BM_MEM_RAM) + if (range_entry_tag(r) == dest_type) return 1; } } @@ -223,9 +224,11 @@ * Returns 1 if the requested memory range is all tagged as type BM_MEM_RAM. * Otherwise returns 0. */ -int bootmem_region_targets_usable_ram(uint64_t start, uint64_t size) +int bootmem_region_targets_usable_ram(uint64_t start, uint64_t size, + enum bootmem_type dest_type) { - return bootmem_region_targets_ram(start, start + size, &bootmem); + return bootmem_region_targets_ram(start, start + size, &bootmem, + dest_type); } void *bootmem_allocate_buffer(size_t size) diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index ac80a62..70ea7ef 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -182,7 +182,7 @@ switch (prog_cbfs_type(payload)) { case CBFS_TYPE_SELF: /* Simple ELF */ - selfload_check(payload); + selfload_check(payload, BM_MEM_RAM); break; case CBFS_TYPE_FIT: /* Flattened image tree */ if (IS_ENABLED(CONFIG_PAYLOAD_FIT_SUPPORT)) { diff --git a/src/lib/selfboot.c b/src/lib/selfboot.c index 9c52cd6..fcdfa0f 100644 --- a/src/lib/selfboot.c +++ b/src/lib/selfboot.c @@ -30,7 +30,7 @@ #include <cbmem.h> /* The type syntax for C is essentially unparsable. -- Rob Pike */ -typedef int (*checker_t)(struct cbfs_payload_segment *cbfssegs); +typedef int (*checker_t)(struct cbfs_payload_segment *cbfssegs, void *args); /* Decode a serialized cbfs payload segment * from memory into native endianness. @@ -46,10 +46,11 @@ segment->mem_len = read_be32(&src->mem_len); } -static int segment_targets_usable_ram(void *dest, unsigned long memsz) +static int segment_targets_usable_ram(void *dest, unsigned long memsz, + enum bootmem_type dest_type) { uintptr_t d = (uintptr_t) dest; - if (bootmem_region_targets_usable_ram(d, memsz)) + if (bootmem_region_targets_usable_ram(d, memsz, dest_type)) return 1; if (payload_arch_usable_ram_quirk(d, memsz)) @@ -140,11 +141,13 @@ return read_be32(&(seg + 1)->type) == PAYLOAD_SEGMENT_ENTRY; } -static int check_payload_segments(struct cbfs_payload_segment *cbfssegs) +static int check_payload_segments(struct cbfs_payload_segment *cbfssegs, + void *args) { uint8_t *dest; size_t memsz; struct cbfs_payload_segment *first_segment, *seg, segment; + enum bootmem_type dest_type = *(enum bootmem_type *)args; for (first_segment = seg = cbfssegs;; ++seg) { printk(BIOS_DEBUG, "Checking segment from ROM address 0x%p\n", seg); @@ -153,7 +156,7 @@ memsz = segment.mem_len; if (segment.type == PAYLOAD_SEGMENT_ENTRY) break; - if (!segment_targets_usable_ram(dest, memsz)) + if (!segment_targets_usable_ram(dest, memsz, dest_type)) return -1; } return 0; @@ -244,7 +247,8 @@ return data; } -static bool _selfload(struct prog *payload, checker_t f) +static bool _selfload(struct prog *payload, checker_t f, + enum bootmem_type dest_type) { uintptr_t entry = 0; struct cbfs_payload_segment *cbfssegs; @@ -256,7 +260,7 @@ cbfssegs = &((struct cbfs_payload *)data)->segments; - if (f && f(cbfssegs)) + if (f && f(cbfssegs, &dest_type)) goto out; if (load_payload_segments(cbfssegs, &entry)) @@ -275,12 +279,12 @@ return false; } -bool selfload_check(struct prog *payload) +bool selfload_check(struct prog *payload, enum bootmem_type dest_type) { - return _selfload(payload, check_payload_segments); + return _selfload(payload, check_payload_segments, dest_type); } bool selfload(struct prog *payload) { - return _selfload(payload, NULL); + return _selfload(payload, NULL, 0); } -- To view, visit https://review.coreboot.org/c/coreboot/+/31122 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iecdeedd9e8da67f73ac47d2a82e85b306469a626 Gerrit-Change-Number: 31122 Gerrit-PatchSet: 1 Gerrit-Owner: Ting Shen <phoenixshen(a)google.com> Gerrit-MessageType: newchange

5 13

Change in ...coreboot[master]: mainboard/{google,intel}: Remove SaGv hard coding
by Patrick Georgi (Code Review) 30 Jan '19

30 Jan '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31120 ) Change subject: mainboard/{google,intel}: Remove SaGv hard coding ...................................................................... mainboard/{google,intel}: Remove SaGv hard coding Remove hard coding for SaGv config in devicetree.cb and apply macro for SaGv config for CNL variants boards Change-Id: If007589d5c1368602928b1550ec8788e65f70c05 Signed-off-by: Ronak Kanabar <ronak.kanabar(a)intel.com> Reviewed-on: https://review.coreboot.org/c/31120 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Reviewed-by: Furquan Shaikh <furquan(a)google.com> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/mainboard/google/sarien/variants/arcada/devicetree.cb M src/mainboard/google/sarien/variants/sarien/devicetree.cb M src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb M src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb M src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb M src/mainboard/intel/coffeelake_rvp/variants/cfl_s/devicetree.cb M src/mainboard/intel/coffeelake_rvp/variants/cfl_u/devicetree.cb M src/mainboard/intel/coffeelake_rvp/variants/whl_u/devicetree.cb 8 files changed, 8 insertions(+), 8 deletions(-) Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Subrata Banik: Looks good to me, but someone else must approve Pratikkumar V Prajapati: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved diff --git a/src/mainboard/google/sarien/variants/arcada/devicetree.cb b/src/mainboard/google/sarien/variants/arcada/devicetree.cb index f9d4582..4efaf55 100644 --- a/src/mainboard/google/sarien/variants/arcada/devicetree.cb +++ b/src/mainboard/google/sarien/variants/arcada/devicetree.cb @@ -14,7 +14,7 @@ register "gen3_dec" = "0x000c0951" # 0x950-0x95f # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "HeciEnabled" = "1" register "SataSalpSupport" = "1" register "SataMode" = "Sata_AHCI" diff --git a/src/mainboard/google/sarien/variants/sarien/devicetree.cb b/src/mainboard/google/sarien/variants/sarien/devicetree.cb index 0bf7e98..85d4f9d 100644 --- a/src/mainboard/google/sarien/variants/sarien/devicetree.cb +++ b/src/mainboard/google/sarien/variants/sarien/devicetree.cb @@ -14,7 +14,7 @@ register "gen3_dec" = "0x000c0951" # 0x950-0x95f # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "HeciEnabled" = "1" register "SataSalpSupport" = "1" register "SataMode" = "Sata_AHCI" diff --git a/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb b/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb index bb963c9..9604210 100644 --- a/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb +++ b/src/mainboard/intel/cannonlake_rvp/variants/cnl_u/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_FixedHigh" register "ScsEmmcHs400Enabled" = "1" register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" diff --git a/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb b/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb index 55afde2..e2ebaba 100644 --- a/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb +++ b/src/mainboard/intel/cannonlake_rvp/variants/cnl_y/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_FixedHigh" register "ScsEmmcHs400Enabled" = "1" register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" diff --git a/src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb b/src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb index 35aa624..9648ac3 100644 --- a/src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb +++ b/src/mainboard/intel/coffeelake_rvp/variants/cfl_h/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "RMT" = "1" register "ScsEmmcHs400Enabled" = "1" diff --git a/src/mainboard/intel/coffeelake_rvp/variants/cfl_s/devicetree.cb b/src/mainboard/intel/coffeelake_rvp/variants/cfl_s/devicetree.cb index bbfc9e7..126cab0 100644 --- a/src/mainboard/intel/coffeelake_rvp/variants/cfl_s/devicetree.cb +++ b/src/mainboard/intel/coffeelake_rvp/variants/cfl_s/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "RMT" = "1" register "ScsEmmcHs400Enabled" = "1" diff --git a/src/mainboard/intel/coffeelake_rvp/variants/cfl_u/devicetree.cb b/src/mainboard/intel/coffeelake_rvp/variants/cfl_u/devicetree.cb index bb963c9..e5f867c 100644 --- a/src/mainboard/intel/coffeelake_rvp/variants/cfl_u/devicetree.cb +++ b/src/mainboard/intel/coffeelake_rvp/variants/cfl_u/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "ScsEmmcHs400Enabled" = "1" register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" diff --git a/src/mainboard/intel/coffeelake_rvp/variants/whl_u/devicetree.cb b/src/mainboard/intel/coffeelake_rvp/variants/whl_u/devicetree.cb index 010ad65..e30da3a 100644 --- a/src/mainboard/intel/coffeelake_rvp/variants/whl_u/devicetree.cb +++ b/src/mainboard/intel/coffeelake_rvp/variants/whl_u/devicetree.cb @@ -5,7 +5,7 @@ end # FSP configuration - register "SaGv" = "3" + register "SaGv" = "SaGv_Enabled" register "ScsEmmcHs400Enabled" = "1" register "HeciEnabled" = "1" -- To view, visit https://review.coreboot.org/c/coreboot/+/31120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If007589d5c1368602928b1550ec8788e65f70c05 Gerrit-Change-Number: 31120 Gerrit-PatchSet: 4 Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Lijian Zhao <lijian.zhao(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in ...coreboot[master]: mb/google/sarien/variants/arcada: Adjust TP/TS/H1 I2C CLK to meet spec
by Casper Chang (Code Review) 30 Jan '19

30 Jan '19

Casper Chang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31124 Change subject: mb/google/sarien/variants/arcada: Adjust TP/TS/H1 I2C CLK to meet spec ...................................................................... mb/google/sarien/variants/arcada: Adjust TP/TS/H1 I2C CLK to meet spec After adjustment on Arcada EVT TouchScreen: 390 KHz TouchPad: 389 KHz H1: 389 KHz BUG=b:120584026, b:120584561 BRANCH=master TEST=emerge-sarien coreboot chromeos-bootimage measure by scope Change-Id: Ia6eb332e7a664b211a5025ad07e0d01bf7f8d5bb --- M src/mainboard/google/sarien/variants/arcada/devicetree.cb 1 file changed, 6 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31124/1 diff --git a/src/mainboard/google/sarien/variants/arcada/devicetree.cb b/src/mainboard/google/sarien/variants/arcada/devicetree.cb index 2b0408e..f9d4582 100644 --- a/src/mainboard/google/sarien/variants/arcada/devicetree.cb +++ b/src/mainboard/google/sarien/variants/arcada/devicetree.cb @@ -77,17 +77,19 @@ .chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT, .i2c[0] = { .speed = I2C_SPEED_FAST, + .rise_time_ns = 52, + .fall_time_ns = 110, }, .i2c[1] = { .speed = I2C_SPEED_FAST, - .rise_time_ns = 176, - .fall_time_ns = 15, + .rise_time_ns = 52, + .fall_time_ns = 110, }, .i2c[4] = { .early_init = 1, .speed = I2C_SPEED_FAST, - .rise_time_ns = 452, - .fall_time_ns = 110, + .rise_time_ns = 36, + .fall_time_ns = 99, }, }" -- To view, visit https://review.coreboot.org/c/coreboot/+/31124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia6eb332e7a664b211a5025ad07e0d01bf7f8d5bb Gerrit-Change-Number: 31124 Gerrit-PatchSet: 1 Gerrit-Owner: Casper Chang <casper_chang(a)wistron.corp-partner.google.com> Gerrit-MessageType: newchange

4 8

Change in ...coreboot[master]: ec/google/chromeec: Add boardid.c to verstage
by You-Cheng Syu (Code Review) 30 Jan '19

30 Jan '19

You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31006 Change subject: ec/google/chromeec: Add boardid.c to verstage ...................................................................... ec/google/chromeec: Add boardid.c to verstage Modifiy Makefile so that we can get board ID in verstage. BRANCH=none BUG=b:117916698 TEST=manually Change-Id: Idcdb6e07f565c937185cab811abac0ce47e5e3a7 Signed-off-by: You-Cheng Syu <youcheng(a)google.com> --- M src/ec/google/chromeec/Makefile.inc 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31006/1 diff --git a/src/ec/google/chromeec/Makefile.inc b/src/ec/google/chromeec/Makefile.inc index 85e2599..4593c73 100644 --- a/src/ec/google/chromeec/Makefile.inc +++ b/src/ec/google/chromeec/Makefile.inc @@ -1,6 +1,7 @@ ifeq ($(CONFIG_EC_GOOGLE_CHROMEEC),y) bootblock-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c +verstage-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c romstage-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c smm-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c -- To view, visit https://review.coreboot.org/c/coreboot/+/31006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idcdb6e07f565c937185cab811abac0ce47e5e3a7 Gerrit-Change-Number: 31006 Gerrit-PatchSet: 1 Gerrit-Owner: You-Cheng Syu <youcheng(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: You-Cheng Syu <youcheng(a)google.com> Gerrit-MessageType: newchange

3 2

Change in ...coreboot[master]: soc/amd/stoneyridge: Access SMBUS through MMIO
by Patrick Georgi (Code Review) 30 Jan '19

30 Jan '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/29258 ) Change subject: soc/amd/stoneyridge: Access SMBUS through MMIO ...................................................................... soc/amd/stoneyridge: Access SMBUS through MMIO Currently SMBUS registers are accessed through IO, but with stoneyridge they can be accessed through MMIO. This reduces the time of execution by a tiny amount (MMIO write is faster than IO write, though MMIO read is about as fast as IO read) as most of the time consumed is actually transaction time. Convert code to MMIO access. BUG=b:117754784 TEST=Used IO to write and MMIO to read, to confirm a one to one relationship between IO and MMIO. Then build and boot grunt. Change-Id: Ibe1471d1d578611e7d666f70bc97de4c3b74d7f8 Signed-off-by: Richard Spiegel <richard.spiegel(a)silverbackltd.com> Reviewed-on: https://review.coreboot.org/c/29258 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Martin Roth <martinroth(a)google.com> --- M src/soc/amd/stoneyridge/include/soc/iomap.h M src/soc/amd/stoneyridge/include/soc/smbus.h M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/sm.c M src/soc/amd/stoneyridge/smbus.c M src/soc/amd/stoneyridge/smbus_spd.c M src/soc/amd/stoneyridge/southbridge.c 8 files changed, 135 insertions(+), 134 deletions(-) Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved diff --git a/src/soc/amd/stoneyridge/include/soc/iomap.h b/src/soc/amd/stoneyridge/include/soc/iomap.h index 78e8e09..b101014 100644 --- a/src/soc/amd/stoneyridge/include/soc/iomap.h +++ b/src/soc/amd/stoneyridge/include/soc/iomap.h @@ -38,6 +38,8 @@ #define PM_MMIO_BASE 0xfed80300 #define BIOSRAM_MMIO_BASE 0xfed80500 #define ACPI_REG_MMIO_BASE 0xfed80800 +#define ASF_MMIO_BASE 0xfed80900 +#define SMBUS_MMIO_BASE 0xfed80a00 #define GPIO_IOMUX_MMIO_BASE 0xfed80d00 #define MISC_MMIO_BASE 0xfed80e00 #define XHCI_ACPI_PM_MMIO_BASE 0xfed81c00 diff --git a/src/soc/amd/stoneyridge/include/soc/smbus.h b/src/soc/amd/stoneyridge/include/soc/smbus.h index 1bb4346..71f7faf 100644 --- a/src/soc/amd/stoneyridge/include/soc/smbus.h +++ b/src/soc/amd/stoneyridge/include/soc/smbus.h @@ -19,50 +19,6 @@ #include <stdint.h> #include <soc/iomap.h> -#define SMBHSTSTAT 0x0 -#define SMBHST_STAT_FAILED 0x10 -#define SMBHST_STAT_COLLISION 0x08 -#define SMBHST_STAT_ERROR 0x04 -#define SMBHST_STAT_INTERRUPT 0x02 -#define SMBHST_STAT_BUSY 0x01 -#define SMBHST_STAT_CLEAR 0xff -#define SMBHST_STAT_NOERROR 0x02 -#define SMBHST_STAT_VAL_BITS 0x1f -#define SMBHST_STAT_ERROR_BITS 0x1c - -#define SMBSLVSTAT 0x1 -#define SMBSLV_STAT_ALERT 0x20 -#define SMBSLV_STAT_SHADOW2 0x10 -#define SMBSLV_STAT_SHADOW1 0x08 -#define SMBSLV_STAT_SLV_STS 0x04 -#define SMBSLV_STAT_SLV_INIT 0x02 -#define SMBSLV_STAT_SLV_BUSY 0x01 -#define SMBSLV_STAT_CLEAR 0x1f - -#define SMBHSTCTRL 0x2 -#define SMBHST_CTRL_RST 0x80 -#define SMBHST_CTRL_STRT 0x40 -#define SMBHST_CTRL_QCK_RW 0x00 -#define SMBHST_CTRL_BTE_RW 0x04 -#define SMBHST_CTRL_BDT_RW 0x08 -#define SMBHST_CTRL_WDT_RW 0x0c -#define SMBHST_CTRL_BLK_RW 0x14 -#define SMBHST_CTRL_MODE_BITS 0x1c -#define SMBHST_CTRL_KILL 0x02 -#define SMBHST_CTRL_IEN 0x01 - -#define SMBHSTCMD 0x3 -#define SMBHSTADDR 0x4 -#define SMBHSTDAT0 0x5 -#define SMBHSTDAT1 0x6 -#define SMBHSTBLKDAT 0x7 -#define SMBSLVCTRL 0x8 -#define SMBSLVCMD_SHADOW 0x9 -#define SMBSLVEVT 0xa -#define SMBSLVDAT 0xc -#define SMBTIMING 0xe - -#define SMB_ASF_IO_BASE 0x01 #define SMB_SPEED_400KHZ (66000000 / (400000 * 4)) #define AX_INDXC 0 @@ -91,10 +47,10 @@ #define rcindxp_reg(reg, port, mask, val) \ alink_rc_indx((RC_INDXP), (reg), (port), (mask), (val)) -int do_smbus_read_byte(u16 smbus_io_base, u8 device, u8 address); -int do_smbus_write_byte(u16 smbus_io_base, u8 device, u8 address, u8 val); -int do_smbus_recv_byte(u16 smbus_io_base, u8 device); -int do_smbus_send_byte(u16 smbus_io_base, u8 device, u8 val); +int do_smbus_read_byte(u32 mmio, u8 device, u8 address); +int do_smbus_write_byte(u32 mmio, u8 device, u8 address, u8 val); +int do_smbus_recv_byte(u32 mmio, u8 device); +int do_smbus_send_byte(u32 mmio, u8 device, u8 val); void alink_rc_indx(u32 reg_space, u32 reg_addr, u32 port, u32 mask, u32 val); void alink_ab_indx(u32 reg_space, u32 reg_addr, u32 mask, u32 val); void alink_ax_indx(u32 space /*c or p? */, u32 axindc, u32 mask, u32 val); diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index adf3af2..1652bbc 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -39,6 +39,8 @@ #define FORCE_SLPSTATE_RETRY BIT(25) #define FORCE_STPCLK_RETRY BIT(24) +#define SMB_ASF_IO_BASE 0x01 /* part of PM_DECODE_EN */ + #define PWR_RESET_CFG 0x10 #define TOGGLE_ALL_PWR_GOOD BIT(1) @@ -111,6 +113,50 @@ #define MMIO_ACPI_GPE0_EN 0x18 #define MMIO_ACPI_PM_TMR_BLK 0x08 +/* SMBUS MMIO offsets 0xfed80a00 */ +#define SMBHSTSTAT 0x0 +#define SMBHST_STAT_FAILED 0x10 +#define SMBHST_STAT_COLLISION 0x08 +#define SMBHST_STAT_ERROR 0x04 +#define SMBHST_STAT_INTERRUPT 0x02 +#define SMBHST_STAT_BUSY 0x01 +#define SMBHST_STAT_CLEAR 0xff +#define SMBHST_STAT_NOERROR 0x02 +#define SMBHST_STAT_VAL_BITS 0x1f +#define SMBHST_STAT_ERROR_BITS 0x1c + +#define SMBSLVSTAT 0x1 +#define SMBSLV_STAT_ALERT 0x20 +#define SMBSLV_STAT_SHADOW2 0x10 +#define SMBSLV_STAT_SHADOW1 0x08 +#define SMBSLV_STAT_SLV_STS 0x04 +#define SMBSLV_STAT_SLV_INIT 0x02 +#define SMBSLV_STAT_SLV_BUSY 0x01 +#define SMBSLV_STAT_CLEAR 0x1f + +#define SMBHSTCTRL 0x2 +#define SMBHST_CTRL_RST 0x80 +#define SMBHST_CTRL_STRT 0x40 +#define SMBHST_CTRL_QCK_RW 0x00 +#define SMBHST_CTRL_BTE_RW 0x04 +#define SMBHST_CTRL_BDT_RW 0x08 +#define SMBHST_CTRL_WDT_RW 0x0c +#define SMBHST_CTRL_BLK_RW 0x14 +#define SMBHST_CTRL_MODE_BITS 0x1c +#define SMBHST_CTRL_KILL 0x02 +#define SMBHST_CTRL_IEN 0x01 + +#define SMBHSTCMD 0x3 +#define SMBHSTADDR 0x4 +#define SMBHSTDAT0 0x5 +#define SMBHSTDAT1 0x6 +#define SMBHSTBLKDAT 0x7 +#define SMBSLVCTRL 0x8 +#define SMBSLVCMD_SHADOW 0x9 +#define SMBSLVEVT 0xa +#define SMBSLVDAT 0xc +#define SMBTIMING 0xe + /* FCH MISC Registers 0xfed80e00 */ #define GPP_CLK_CNTRL 0x00 #define GPP_CLK2_REQ_MAP_SHIFT 8 @@ -480,6 +526,8 @@ uint16_t xhci_pm_read16(uint8_t reg); void xhci_pm_write32(uint8_t reg, uint32_t value); uint32_t xhci_pm_read32(uint8_t reg); +void smbus_write8(uint32_t mmio, uint8_t reg, uint8_t value); +uint8_t smbus_read8(uint32_t mmio, uint8_t reg); void bootblock_fch_early_init(void); void bootblock_fch_init(void); /** diff --git a/src/soc/amd/stoneyridge/sb_util.c b/src/soc/amd/stoneyridge/sb_util.c index ccbde7e..eeb550c 100644 --- a/src/soc/amd/stoneyridge/sb_util.c +++ b/src/soc/amd/stoneyridge/sb_util.c @@ -200,6 +200,16 @@ return read32((void *)(XHCI_ACPI_PM_MMIO_BASE + reg)); } +void smbus_write8(uint32_t mmio, uint8_t reg, uint8_t value) +{ + write8((void *)(mmio + reg), value); +} + +uint8_t smbus_read8(uint32_t mmio, uint8_t reg) +{ + return read8((void *)(mmio + reg)); +} + int acpi_get_sleep_type(void) { return acpi_sleep_from_pm1(inw(pm_acpi_pm_cnt_blk())); diff --git a/src/soc/amd/stoneyridge/sm.c b/src/soc/amd/stoneyridge/sm.c index 22cbde8..82c265e 100644 --- a/src/soc/amd/stoneyridge/sm.c +++ b/src/soc/amd/stoneyridge/sm.c @@ -35,60 +35,49 @@ setup_ioapic(VIO_APIC_VADDR, CONFIG_MAX_CPUS); } -static int lsmbus_recv_byte(struct device *dev) +static u32 get_sm_mmio(struct device *dev) { - u8 device; struct resource *res; struct bus *pbus; - device = dev->path.i2c.device; pbus = get_pbus_smbus(dev); - res = find_resource(pbus->dev, 0x90); + if (res->base == SMB_BASE_ADDR) + return SMBUS_MMIO_BASE; - return do_smbus_recv_byte(res->base, device); + return ASF_MMIO_BASE; +} + +static int lsmbus_recv_byte(struct device *dev) +{ + u8 device; + + device = dev->path.i2c.device; + return do_smbus_recv_byte(get_sm_mmio(dev), device); } static int lsmbus_send_byte(struct device *dev, u8 val) { u8 device; - struct resource *res; - struct bus *pbus; device = dev->path.i2c.device; - pbus = get_pbus_smbus(dev); - - res = find_resource(pbus->dev, 0x90); - - return do_smbus_send_byte(res->base, device, val); + return do_smbus_send_byte(get_sm_mmio(dev), device, val); } static int lsmbus_read_byte(struct device *dev, u8 address) { u8 device; - struct resource *res; - struct bus *pbus; device = dev->path.i2c.device; - pbus = get_pbus_smbus(dev); - - res = find_resource(pbus->dev, 0x90); - - return do_smbus_read_byte(res->base, device, address); + return do_smbus_read_byte(get_sm_mmio(dev), device, address); } static int lsmbus_write_byte(struct device *dev, u8 address, u8 val) { u8 device; - struct resource *res; - struct bus *pbus; device = dev->path.i2c.device; - pbus = get_pbus_smbus(dev); - - res = find_resource(pbus->dev, 0x90); - - return do_smbus_write_byte(res->base, device, address, val); + return do_smbus_write_byte(get_sm_mmio(dev), device, address, val); } static struct smbus_bus_operations lops_smbus_bus = { .recv_byte = lsmbus_recv_byte, diff --git a/src/soc/amd/stoneyridge/smbus.c b/src/soc/amd/stoneyridge/smbus.c index 919a52e..21750d8 100644 --- a/src/soc/amd/stoneyridge/smbus.c +++ b/src/soc/amd/stoneyridge/smbus.c @@ -16,143 +16,144 @@ #include <io.h> #include <stdint.h> #include <soc/smbus.h> +#include <soc/southbridge.h> -static int smbus_wait_until_ready(u16 smbus_io_base) +static int smbus_wait_until_ready(u32 mmio) { u32 loops; loops = SMBUS_TIMEOUT; do { u8 val; - val = inb(smbus_io_base + SMBHSTSTAT); + val = smbus_read8(mmio, SMBHSTSTAT); val &= SMBHST_STAT_VAL_BITS; if (val == 0) { /* ready now */ return 0; } - outb(val, smbus_io_base + SMBHSTSTAT); + smbus_write8(mmio, SMBHSTSTAT, val); } while (--loops); return -2; /* time out */ } -static int smbus_wait_until_done(u16 smbus_io_base) +static int smbus_wait_until_done(u32 mmio) { u32 loops; loops = SMBUS_TIMEOUT; do { u8 val; - val = inb(smbus_io_base + SMBHSTSTAT); + val = smbus_read8(mmio, SMBHSTSTAT); val &= SMBHST_STAT_VAL_BITS; /* mask off reserved bits */ if (val & SMBHST_STAT_ERROR_BITS) return -5; /* error */ if (val == SMBHST_STAT_NOERROR) { - outb(val, smbus_io_base + SMBHSTSTAT); /* clear sts */ + smbus_write8(mmio, SMBHSTSTAT, val); /* clear sts */ return 0; } } while (--loops); return -3; /* timeout */ } -int do_smbus_recv_byte(u16 smbus_io_base, u8 device) +int do_smbus_recv_byte(u32 mmio, u8 device) { u8 byte; - if (smbus_wait_until_ready(smbus_io_base) < 0) + if (smbus_wait_until_ready(mmio) < 0) return -2; /* not ready */ /* set the device I'm talking to */ - outb(((device & 0x7f) << 1) | 1, smbus_io_base + SMBHSTADDR); + smbus_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 1); - byte = inb(smbus_io_base + SMBHSTCTRL); + byte = smbus_read8(mmio, SMBHSTCTRL); byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BTE_RW; /* set mode, start */ - outb(byte, smbus_io_base + SMBHSTCTRL); + smbus_write8(mmio, SMBHSTCTRL, byte); /* poll for transaction completion */ - if (smbus_wait_until_done(smbus_io_base) < 0) + if (smbus_wait_until_done(mmio) < 0) return -3; /* timeout or error */ /* read results of transaction */ - byte = inb(smbus_io_base + SMBHSTDAT0); + byte = smbus_read8(mmio, SMBHSTDAT0); return byte; } -int do_smbus_send_byte(u16 smbus_io_base, u8 device, u8 val) +int do_smbus_send_byte(u32 mmio, u8 device, u8 val) { u8 byte; - if (smbus_wait_until_ready(smbus_io_base) < 0) + if (smbus_wait_until_ready(mmio) < 0) return -2; /* not ready */ /* set the command... */ - outb(val, smbus_io_base + SMBHSTDAT0); + smbus_write8(mmio, SMBHSTDAT0, val); /* set the device I'm talking to */ - outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBHSTADDR); + smbus_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 0); - byte = inb(smbus_io_base + SMBHSTCTRL); + byte = smbus_read8(mmio, SMBHSTCTRL); byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BTE_RW; /* set mode, start */ - outb(byte, smbus_io_base + SMBHSTCTRL); + smbus_write8(mmio, SMBHSTCTRL, byte); /* poll for transaction completion */ - if (smbus_wait_until_done(smbus_io_base) < 0) + if (smbus_wait_until_done(mmio) < 0) return -3; /* timeout or error */ return 0; } -int do_smbus_read_byte(u16 smbus_io_base, u8 device, u8 address) +int do_smbus_read_byte(u32 mmio, u8 device, u8 address) { u8 byte; - if (smbus_wait_until_ready(smbus_io_base) < 0) + if (smbus_wait_until_ready(mmio) < 0) return -2; /* not ready */ /* set the command/address... */ - outb(address & 0xff, smbus_io_base + SMBHSTCMD); + smbus_write8(mmio, SMBHSTCMD, address & 0xff); /* set the device I'm talking to */ - outb(((device & 0x7f) << 1) | 1, smbus_io_base + SMBHSTADDR); + smbus_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 1); - byte = inb(smbus_io_base + SMBHSTCTRL); + byte = smbus_read8(mmio, SMBHSTCTRL); byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BDT_RW; /* set mode, start */ - outb(byte, smbus_io_base + SMBHSTCTRL); + smbus_write8(mmio, SMBHSTCTRL, byte); /* poll for transaction completion */ - if (smbus_wait_until_done(smbus_io_base) < 0) + if (smbus_wait_until_done(mmio) < 0) return -3; /* timeout or error */ /* read results of transaction */ - byte = inb(smbus_io_base + SMBHSTDAT0); + byte = smbus_read8(mmio, SMBHSTDAT0); return byte; } -int do_smbus_write_byte(u16 smbus_io_base, u8 device, u8 address, u8 val) +int do_smbus_write_byte(u32 mmio, u8 device, u8 address, u8 val) { u8 byte; - if (smbus_wait_until_ready(smbus_io_base) < 0) + if (smbus_wait_until_ready(mmio) < 0) return -2; /* not ready */ /* set the command/address... */ - outb(address & 0xff, smbus_io_base + SMBHSTCMD); + smbus_write8(mmio, SMBHSTCMD, address & 0xff); /* set the device I'm talking to */ - outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBHSTADDR); + smbus_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 0); /* output value */ - outb(val, smbus_io_base + SMBHSTDAT0); + smbus_write8(mmio, SMBHSTDAT0, val); - byte = inb(smbus_io_base + SMBHSTCTRL); + byte = smbus_read8(mmio, SMBHSTCTRL); byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BDT_RW; /* set mode, start */ - outb(byte, smbus_io_base + SMBHSTCTRL); + smbus_write8(mmio, SMBHSTCTRL, byte); /* poll for transaction completion */ - if (smbus_wait_until_done(smbus_io_base) < 0) + if (smbus_wait_until_done(mmio) < 0) return -3; /* timeout or error */ return 0; diff --git a/src/soc/amd/stoneyridge/smbus_spd.c b/src/soc/amd/stoneyridge/smbus_spd.c index 384bc8a..63b457c 100644 --- a/src/soc/amd/stoneyridge/smbus_spd.c +++ b/src/soc/amd/stoneyridge/smbus_spd.c @@ -27,8 +27,7 @@ * sending offset for every byte. * Reads 128 bytes in 7-8 ms at 400 KHz. */ -static int readspd(uint16_t iobase, uint8_t SmbusSlaveAddress, - char *buffer, size_t count) +static int readspd(uint8_t SmbusSlaveAddress, char *buffer, size_t count) { uint8_t dev_addr; size_t index; @@ -36,8 +35,8 @@ char *pbuf = buffer; printk(BIOS_SPEW, "-------------READING SPD-----------\n"); - printk(BIOS_SPEW, "iobase: 0x%08X, SmbusSlave: 0x%08X, count: %d\n", - iobase, SmbusSlaveAddress, (int)count); + printk(BIOS_SPEW, "SmbusSlave: 0x%08X, count: %zd\n", + SmbusSlaveAddress, count); /* * Convert received device address to the format accepted by @@ -46,7 +45,7 @@ dev_addr = (SmbusSlaveAddress >> 1); /* Read the first SPD byte */ - error = do_smbus_read_byte(iobase, dev_addr, 0); + error = do_smbus_read_byte(SMBUS_MMIO_BASE, dev_addr, 0); if (error < 0) { printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n"); return error; @@ -56,7 +55,7 @@ /* Read the remaining SPD bytes using do_smbus_recv_byte for speed */ for (index = 1 ; index < count ; index++) { - error = do_smbus_recv_byte(iobase, dev_addr); + error = do_smbus_recv_byte(SMBUS_MMIO_BASE, dev_addr); if (error < 0) { printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n"); return error; @@ -70,24 +69,7 @@ return 0; } -static void write_pm_reg(int reg, int data) -{ - outb(reg, PM_INDEX); - outb(data, PM_DATA); -} - -static void setup_fch(uint16_t ioBase) -{ - write_pm_reg(SMB_ASF_IO_BASE, ioBase >> 8); - outb(SMB_SPEED_400KHZ, ioBase + SMBTIMING); - /* Clear all SMBUS status bits */ - outb(SMBHST_STAT_CLEAR, ioBase + SMBHSTSTAT); - outb(SMBSLV_STAT_CLEAR, ioBase + SMBSLVSTAT); -} - int sb_read_spd(uint8_t spdAddress, char *buf, size_t len) { - uint16_t ioBase = SMB_BASE_ADDR; - setup_fch(ioBase); - return readspd(ioBase, spdAddress, buf, len); + return readspd(spdAddress, buf, len); } diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index eb41882..c8d66ac 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -28,6 +28,7 @@ #include <amdblocks/agesawrapper.h> #include <amdblocks/reset.h> #include <soc/southbridge.h> +#include <soc/smbus.h> #include <soc/smi.h> #include <soc/amd_pci_int_defs.h> #include <delay.h> @@ -619,6 +620,17 @@ } } +static void fch_smbus_init(void) +{ + pm_write8(SMB_ASF_IO_BASE, SMB_BASE_ADDR >> 8); + smbus_write8(SMBUS_MMIO_BASE, SMBTIMING, SMB_SPEED_400KHZ); + /* Clear all SMBUS status bits */ + smbus_write8(SMBUS_MMIO_BASE, SMBHSTSTAT, SMBHST_STAT_CLEAR); + smbus_write8(SMBUS_MMIO_BASE, SMBSLVSTAT, SMBSLV_STAT_CLEAR); + smbus_write8(ASF_MMIO_BASE, SMBHSTSTAT, SMBHST_STAT_CLEAR); + smbus_write8(ASF_MMIO_BASE, SMBSLVSTAT, SMBSLV_STAT_CLEAR); +} + /* Before console init */ void bootblock_fch_early_init(void) { @@ -631,6 +643,7 @@ sb_spibase(); sb_disable_4dw_burst(); /* Must be disabled on CZ(ST) */ sb_acpi_mmio_decode(); + fch_smbus_init(); sb_enable_cf9_io(); setup_spread_spectrum(&reboot); setup_misc(&reboot); -- To view, visit https://review.coreboot.org/c/coreboot/+/29258 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibe1471d1d578611e7d666f70bc97de4c3b74d7f8 Gerrit-Change-Number: 29258 Gerrit-PatchSet: 13 Gerrit-Owner: Richard Spiegel <richard.spiegel(a)silverbackltd.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Richard Spiegel <richard.spiegel(a)silverbackltd.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in ...coreboot[master]: siemens/mc_apl2: Change SERIRQ mode
by Mario Scheithauer (Code Review) 30 Jan '19

30 Jan '19

Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31138 Change subject: siemens/mc_apl2: Change SERIRQ mode ...................................................................... siemens/mc_apl2: Change SERIRQ mode Because of Intel's faulty LPC clock, the SERIRQ mode must be corrected. By removing this entry from devicetree, the default value (quiet mode) is used. Change-Id: I7a45e0e5fcde17a20abd19a33282b8a9215b1480 Signed-off-by: Mario Scheithauer <mario.scheithauer(a)siemens.com> --- M src/mainboard/siemens/mc_apl1/variants/mc_apl2/devicetree.cb 1 file changed, 0 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/31138/1 diff --git a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/devicetree.cb b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/devicetree.cb index e54444a..c362e6c 100644 --- a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/devicetree.cb +++ b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/devicetree.cb @@ -5,7 +5,6 @@ end register "sci_irq" = "SCIS_IRQ10" - register "serirq_mode" = "SERIRQ_CONTINUOUS" # Disable all clkreq of PCIe root ports as SMARC interface do not # have this pins. -- To view, visit https://review.coreboot.org/c/coreboot/+/31138 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7a45e0e5fcde17a20abd19a33282b8a9215b1480 Gerrit-Change-Number: 31138 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-MessageType: newchange

5 9

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2019