might be good to have the added defines and the change in the board's devicetree in separate patches. I'm not sure if it would be better to have the defines in the chip.h or in the gfx.h file; don't know that part of the code base well enough to be able to say for sure which would be the better location

Patch set 2:Code-Review +1

2 comments:

File src/soc/intel/braswell/include/soc/gfx.h:
- Patch Set #2, Line 20:
```
#define IGD_MEMSIZE_32MB	0x01
#define IGD_MEMSIZE_64MB	0x02
#define IGD_MEMSIZE_96MB	0x03
#define IGD_MEMSIZE_128MB	0x04
```
  maybe add a comment that those values are for the configuration in the devicetree? or should these rather be in the chip.h file, since those are only for the interface to the FSP? see also my comment below
- Patch Set #2, Line 38:
```
# define GGC_GSM_SIZE_32MB	(1 << 3)
# define GGC_GSM_SIZE_64MB	(2 << 3)
# define GCC_GSM_SIZE_96MB	(3 << 3)
# define GGC_GSM_SIZE_128MB	(4 << 3)
```
  aren't those just shifted versions of the new defines above? or is the one above for a completely different API that happens to just put the values in the hardware registers in the end?

To view, visit change 35064. To unsubscribe, or for help writing mail filters, visit settings.