[U-Boot] [PATCH] x86: WIP: Enable D0 stepping microcode for MinnowMax

[Andrew Bradford]

Hi Simon,

On 08/06 08:02, Simon Glass wrote:
> Hi,
> 
> On 29 June 2015 at 11:57, Simon Glass <sjg at chromium.org> wrote:
> > Hi Andrew,
> >
> > On 29 June 2015 at 08:36, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> >> Hi Simon,
> >>
> >> On 06/07 18:58, Simon Glass wrote:
> >>> Hi Bin,
> >>>
> >>> On 5 June 2015 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> >>> > Hi Simon,
> >>> >
> >>> > On Sat, Jun 6, 2015 at 12:17 AM, Simon Glass <sjg at chromium.org> wrote:
> >>> >> Hi Bin,
> >>> >>
> >>> >> On 4 June 2015 at 20:03, Bin Meng <bmeng.cn at gmail.com> wrote:
> >>> >>> Hi Simon,
> >>> >>>
> >>> >>> On Fri, Jun 5, 2015 at 2:31 AM, Simon Glass <sjg at chromium.org> wrote:
> >>> >>>> Hi,
> >>> >>>>
> >>> >>>> On 4 June 2015 at 10:27, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> >>> >>>>>
> >>> >>>>> Hi Bin,
> >>> >>>>>
> >>> >>>>> On 06/04 22:21, Bin Meng wrote:
> >>> >>>>> > Hi Simon,
> >>> >>>>> >
> >>> >>>>> > On Thu, Jun 4, 2015 at 8:12 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> >>> >>>>> > > This is a temparory hacking for testing U-Boot on a newer version
> >>> >>>>> > > MinnowMax board.
> >>> >>>>> > >
> >>> >>>>> > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >>> >>>>> > > ---
> >>> >>>>> > >
> >>> >>>>> > >  arch/x86/dts/minnowmax.dts | 2 +-
> >>> >>>>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >>> >>>>> > >
> >>> >>>>> > > diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
> >>> >>>>> > > index 7103bc5..9e1fcfc 100644
> >>> >>>>> > > --- a/arch/x86/dts/minnowmax.dts
> >>> >>>>> > > +++ b/arch/x86/dts/minnowmax.dts
> >>> >>>>> > > @@ -101,7 +101,7 @@
> >>> >>>>> > >
> >>> >>>>> > >         microcode {
> >>> >>>>> > >                 update at 0 {
> >>> >>>>> > > -#include "microcode/m0130673322.dtsi"
> >>> >>>>> > > +#include "microcode/m0130679901.dtsi"
> >>> >>>>> > >                 };
> >>> >>>>> > >         };
> >>> >>>>> > >
> >>> >>>>> > > --
> >>> >>>>> >
> >>> >>>>> > Saket confirmed these two patches resolved the boot problem he saw. So
> >>> >>>>> > we will need think about how to better support such scenario that
> >>> >>>>> > different revision board with different stepping CPUs. Could be
> >>> >>>>> > multiple microcodes in one U-Boot image, or still one microcode with
> >>> >>>>> > some #ifdef #endif. Note FSP has the capability to accept multiple
> >>> >>>>> > microcodes as parameters in the FspTempRamInit(), but right now U-Boot
> >>> >>>>> > only specifies one. How do you think?
> >>> >>>>>
> >>> >>>>> Why not just have a minnowmax common dtsi and then top level dts files
> >>> >>>>> for each revision of the board containing the ways they are different
> >>> >>>>> (such as microcode)?
> >>> >>>>
> >>> >>>> My preference would be to include all the microcode needed by the
> >>> >>>> board and then pass the correct one to the FSP. Now that we can access
> >>> >>>> the device tree that should be possible. There is logic in
> >>> >>>> ./arch/x86/cpu/ivybridge/microcode_intel.c to do this but it may need
> >>> >>>> a bit of refactorng.
> >>> >>>>
> >>> >>>
> >>> >>> The device tree still cannot be accessed in that early phase before
> >>> >>> CAR is initialized. The issue is that FSP is designed to have
> >>> >>> FspTempRamInit() to do both microcode loading and CAR initialization.
> >>> >>>
> >>> >>
> >>> >> Ah yes of course. Most unfortunate.
> >>> >>
> >>> >> Maybe we could have ifdtool or the Makefile put all the microcode in a
> >>> >> single big blob somewhere else in u-boot.rom so that the whole thing
> >>> >> can be passed to the FSP. In that case I think the FSP will select the
> >>> >> correct one
> >>> >
> >>> > Maybe an easy way to handle this is to create a special microcode
> >>> > section in u-boot.lds and just include all these header files in a
> >>> > single array in the section. This does not need ifdtool or device tree
> >>> > support.
> >>>
> >>> I'd prefer to keep it in the device tree if we can. It is easier to
> >>> read. For non-FSP targets this works fine and I live in hope that
> >>> Intel might improve the FSP interface such that the single microcode
> >>> blob is not necessary.
> >>
> >> Are you saying that then for a board that could have more than one
> >> microcode used that the device tree for said board would actually list
> >> two different dtsi microcode files and then have a handler that is able
> >> to pass both to the FSP?
> >>
> >> I don't get the impression that Intel is going to improve the FSP for
> >> Baytrail at this point.
> >>
> >> I'd like to get Bin's microcode patch included into u-boot so it can be
> >> used with newer stepping E3800 parts, what's the best way I can help?
> >>
> >
> > My understanding is that you can pass several microcode blobs to the
> > FSP and it will load the correct one (as Bin says above). If that is
> > untrue then this will not work.
> >
> > My preference is that we modify ifdtool to support collecting the
> > microcode blobs together and put them into a single place in the ROM.
> > This could be run from the Makefile as another ifdtool step. It would
> > find the various microcode nodes, extract each blob of binary data,
> > pack them together and put them in a specified place in ROM.
> 
> Any thoughts on this? I don't actually have a newer board to test
> with. But I suspect that no one has both an old and a new board.

I think it's a good idea, I just haven't had any time to look at it,
sorry.  My custom E3800 based boards are all D0 stepping, I believe, so
I've just been using Bin's D0 stepping patch for the work I've been
doing with them.

Thanks,
Andrew