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

[Simon Glass]

Hi Andrew,

On 12 August 2015 at 06:36, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> 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?

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