Hi Andrew, On 12 August 2015 at 06:36, Andrew Bradford <[email protected]> wrote: > Hi Simon, > > On 08/07 20:27, Bin Meng wrote: >> Hi Andrew, >> >> On Fri, Aug 7, 2015 at 8:11 PM, Andrew Bradford >> <[email protected]> wrote: >> > Hi Bin, >> > >> > On 08/07 08:23, Bin Meng wrote: >> >> Hi Andrew, >> >> >> >> On Fri, Aug 7, 2015 at 4:08 AM, Andrew Bradford >> >> <[email protected]> wrote: >> >> > From: Andrew Bradford <[email protected]> >> >> > >> >> > 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 <[email protected]> >> >> > --- >> >> >> >> Reviewed-by: Bin Meng <[email protected]> >> >> Tested-by: Bin Meng <[email protected]> >> >> >> >> 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([email protected]): 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. > > Any thoughts on the best way to list out the magic numbers for Bay Trail > FSP? > > Should there be two different header files (one for dts and one for > code) like Quark MRC does it or just a single header file which is used > by both dts and code?
I don't think we need any duplication - we should include the binding file from C code also. This helps keep the C and device tree in sync. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
