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

[Bin Meng]

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.

> 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