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

[Andrew Bradford]

Hi Simon,

On 07/21 14:17, Simon Glass wrote:
> Hi Andrew,
> 
> On 10 July 2015 at 12:24, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> > Hi Simon,
> >
> > On 07/10 06:53, Simon Glass wrote:
> >> Hi,
> >>
> >> On 8 July 2015 at 05:30, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> >> > Hi Bin,
> >> >
> >> > On 07/08 11:18, Bin Meng wrote:

<snip>

> >> >> My understanding is that we either use device tree or the FSP
> >> >> defaults. So if we describe an FSP node in device tree, then we go
> >> >> with device tree as U-Boot's configuration. We cannot use partial
> >> >> configuration from device tree and partial from FSP defaults. That's
> >> >> confusing.
> >> >
> >> > I agree that it could be confusing, even just in writing this patch and
> >> > trying to test it I've run into the fact that in order to see if I'm
> >> > properly overriding the FSP's configuration that I would need to build a
> >> > bunch of different FSP configurations in Intel's Binary Configuration
> >> > Tool.  I've not done this as it would be extremely time consuming.
> >> >
> >> > I'm open to making the properties into bools and then overriding all of
> >> > the FSP's UPD fields with sane defaults if the device tree does not
> >> > specify a non-bool property.  I now think this might be a more sane
> >> > approach for the long run, but I would need some assistance to know what
> >> > the proper settings are for MinnowMax as I do not have one to test with
> >> > and I do not know the full backstory as to why the FSP that shipped with
> >> > MinnowMax was found to need to have its UPD overridden prior to my
> >> > patch.
> >>
> >> It is mostly that the FSP can be set to anything since it is
> >> configured by a proprietary binary tool. Having configuration in two
> >> places, particularly where one is uncertain, does seem confusing.
> >>
> >> So I think Bin's proposal is that we either override everything and
> >> don't use the FSP config, or we override nothing and use the entire
> >> FSP config.
> >
> > Yes, I agree, I think this will be the easiest to understand way.
> >
> > What I'll do for v3 is to make all of the "enable" properties into
> > bools.
> 
> OK
> 
> >
> > For the properties which are ints when the device tree does not have a
> > specification for the property, like dimm-width, do you think it
> > is better to trust the FSP's compiled in value or to simply set a sane
> > default value in fdtdec_get_int()?
> >
> >> Re the Minnowmax settings, I can dump out the list of current settings
> >> if you like.
> >
> > Yes, please.  This would be very helpful.
> 
> Sorry this has taken a while. It is quite painful. Here is my manual
> decoding of fsp.bin. I did it this way you can decode a different
> version, which might have things in different places. You can follow
> along with a hex editor. I suppose you could also write a little tool
> to display the UPD values.
> 
> UPD
> FSP binary data
> 
> see find_fsp_header()
> 
> 0 struct fv_header
> u16 ext_hdr_off at 0x38 = 0x0060
> 0 + 0x60 = 0x60
> 
> 0x60 has struct fv_ext_header
> ext_hdr_size at 0x70 = 0x14
> 0x60 + 0x14 = 0x74
> round up to multiple of 8 bytes = 0x78
> 
> 0x78 has struct ffs_file_header
> 16 byte guid at 0x78  - FSP_GUID_DATA1, etc.
> 
> sizeof(struct ffs_file_header) = 0x18
> 0x78 + 0x18 = 0x90
> 
> 0x90 has struct raw_section
> ->type at 0x93 = 0x19
> sizeof(struct raw_section) = 4
> 0x90 + 4 = 0x94
> 
> struct fsp_header at 0x94
> 0x94 has 'FSPH'
> 
> ->img_base at offset 0x1c = 0xb0
> 0xb0 has 0xfffc0000
> 
> ->cfg_region_off at offset 0x24 = 0xb8
> 0xb8 has 0x9dac
> 
> ->cfg_region_size at offset 0x28 = 0xbc
> 0xbc has 0x36
> 
> struct struct vpd_region at 0x9dac
> ->sign at offset 0 = 0x9dac
>   "VLYVIEW1"
> ->img_rev at offset 8 = 0x9db4
>   0x303
> 
> ->upd_offset at offset 0xc = 9x9db8
>   -0x1f494
> 
> struct upd_region at 0x1f494
> ->signature = "VLV2UPDR"
> data as per struct upd_region
> 
> extends from 0x1f494 to 0x1f596 as per struct upd_region (ends with
> bytes 0xaa, 0x55)
> 00000000  56 4c 56 32 55 50 44 52  e4 ff ff ff 00 00 00 00  |VLV2UPDR........|
> 00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> 00000020  01 00 00 08 a0 a2 02 01  01 00 01 01 01 01 01 00  |................|
> 00000030  00 00 00 00 01 01 01 01  01 01 01 01 01 01 01 01  |................|
> 00000040  01 01 00 02 02 02 f8 03  00 00 01 00 00 01 00 00  |................|
> 00000050  04 01 00 08 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> 00000060  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 000000f0  00 02 01 01 00 00 01 03  00 09 09 0a 05 04 05 14  |................|
> 00000100  aa 55                                             |.U|
> 00000102

<snip>

Thanks! :)

I may not be able to send v3 of this patch until next week due to other
obligations.

Thanks for your help!
-Andrew