Hi Andrew, On Fri, Aug 7, 2015 at 8:11 PM, Andrew Bradford <and...@bradfordembedded.com> wrote: > Hi Bin, > > On 08/07 08:23, Bin Meng wrote: >> Hi Andrew, >> >> On Fri, Aug 7, 2015 at 4:08 AM, Andrew Bradford >> <and...@bradfordembedded.com> wrote: >> > From: Andrew Bradford <andrew.bradf...@kodakalaris.com> >> > >> > Allow for configuration of FSP UPD from the device tree which will >> > override any settings which the FSP was built with itself. >> > >> > Modify the MinnowMax and BayleyBay boards to transfer sensible UPD >> > settings from the Intel FSPv4 Gold release to the respective dts files, >> > with the condition that the memory-down parameters for MinnowMax are >> > also used. >> > >> >> Thanks for updating BayleyBay dts as well :) > > You're welcome :) > My custom board actually boots with the Bayley Bay configuration, once I > change out the dts to use your D0 stepping microcode patch, so that was > a good check for me. > >> > Signed-off-by: Andrew Bradford <andrew.bradf...@kodakalaris.com> >> > --- >> >> Reviewed-by: Bin Meng <bmeng...@gmail.com> >> Tested-by: Bin Meng <bmeng...@gmail.com> >> >> But please see some comments/nits below. > > Would it be best if I fix your comments/nits and send a v4?
Yep, please send a v4 to address these. > >> > >> > Changes from v2: >> > - Switch to using booleans in dts, where appropriate. >> > - Add Bayley Bay dts modifications. >> > - Clarify docs to show bool versus int properties. >> > - Include enable-igt property from FSPv4. >> > >> > Changes from v1: >> > - Use "-" rather than "_" in dt property names. >> > - Use "Bay Trail" for the formal name of the Intel product family. >> > - Use an "fsp," prefix for dt property names for clarity. >> > - Fix minor code indentation issues. >> > - Create a dt subnode for the memory-down-params. >> > - Clarify documentation that dt overrides the FSP's config, so we don't >> > use booleans. >> > [snip] >> > - /* >> > - * Configure everything here to avoid the poor hard-pressed user >> > - * needing to run Intel's binary configuration tool. It may also >> > allow >> > - * us to support the 1GB single core variant easily. >> > - * >> > - * TODO(s...@chromium.org): Move to device tree >> > - */ >> > - fsp_upd->mrc_init_tseg_size = 8; >> > - fsp_upd->mrc_init_mmio_size = 0x800; >> >> One general comment: I think we should replace these hardcoded numbers >> with meaningful macros (eg: 0x800 means MMIO uses 2GB space, that's >> why we see previous the pci does not work on a 4GB DIMM as FSP only >> maps 2GB RAM per this setting). See Intel's Baytrail_FSP_Gold4.tgz >> release FSP/BayleyBayFsp.bsf file, it has the details of how each >> magic number maps to what meaning. Will you do a follow-up patch for >> this? I can do that as well. > > I can do this. > Thanks! > For Quark, I see that there's both include/dt-bindings/mrc/quark.h (for > the dts to use) and arch/x86/include/asm/arch-quark/mrc.h (for code to > use). These two files seem to duplicate some of the same #defines. For > the FSP on Bay Trail, would it be recommended to follow a similar pair > of header files as are used for MRC on Quark or should there just be a > single header file defining the FSP magic numbers? > I am not sure. Maybe Simon can comment. > Regarding the MMIO region, the other choices in the FSP are 1 GB, 1.5 > GB, and 2 GB. Any of those choices will cause issues when the system > has 4 GB of SDRAM, unless I'm mistaken. Although I'm fairly sure we > have some u-boot things hard coded to 0x80000000 for the RAM top in 32 > bit mode, so changing that FSP configuration may cause issues... Sorry but I did not test the MMIO region with 1G/1.5G configuration. When I was checking the bsf file for the magic number 0x800, it occurred to me that issue you previously met :) [snip] > Thanks for the review! :) You are welcome. Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
