[U-Boot] [PATCH v3] x86: baytrail: Configure FSP UPD from device tree

[Andrew Bradford]

Hi Simon,

On 08/07 20:27, Bin Meng wrote:
> Hi Andrew,
> 
> On Fri, Aug 7, 2015 at 8:11 PM, Andrew Bradford
> <andrew at 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
> >> <andrew at bradfordembedded.com> wrote:
> >> > From: Andrew Bradford <andrew.bradford at 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.bradford at kodakalaris.com>
> >> > ---
> >>
> >> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> >> Tested-by: Bin Meng <bmeng.cn at 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(sjg at 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.

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?

Thanks,
Andrew