| Message ID | 1438939726-4781-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: > Currently the microcode-tool writes microcode into a data block as > well as the device tree properties which represents the first 48 > bytes in the microcode data. Now we change the tool to only write > the microcode without device tree stuff so that multiple microcode > data blocks can be included in a single property. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > tools/microcode-tool.py | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) I would rather than we use a tool to pack the microcode together (e.g. ifdtool) rather than changing the source data. I realise that the FSP requires this packing, but I quite dislike the approach of making the source files fit the object files. If you like I can take a look at adding this feature to ifdtool. Regards, Simon
Hi Simon, On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >> Currently the microcode-tool writes microcode into a data block as >> well as the device tree properties which represents the first 48 >> bytes in the microcode data. Now we change the tool to only write >> the microcode without device tree stuff so that multiple microcode >> data blocks can be included in a single property. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> tools/microcode-tool.py | 23 ++++++++++++----------- >> 1 file changed, 12 insertions(+), 11 deletions(-) > > I would rather than we use a tool to pack the microcode together (e.g. > ifdtool) rather than changing the source data. I realise that the FSP > requires this packing, but I quite dislike the approach of making the > source files fit the object files. > Could you roughly describe how you want to do using ifdtool? Is the microcode source still from dtb? > If you like I can take a look at adding this feature to ifdtool. > Regards, Bin
Hi Bin, On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Currently the microcode-tool writes microcode into a data block as >>> well as the device tree properties which represents the first 48 >>> bytes in the microcode data. Now we change the tool to only write >>> the microcode without device tree stuff so that multiple microcode >>> data blocks can be included in a single property. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> tools/microcode-tool.py | 23 ++++++++++++----------- >>> 1 file changed, 12 insertions(+), 11 deletions(-) >> >> I would rather than we use a tool to pack the microcode together (e.g. >> ifdtool) rather than changing the source data. I realise that the FSP >> requires this packing, but I quite dislike the approach of making the >> source files fit the object files. >> > > Could you roughly describe how you want to do using ifdtool? Is the > microcode source still from dtb? Yes I think it can be done by adding an option to generate the microcode blob (since for non-FSP platforms it is unnecessary). It can scan the available microcode nodes and pack them into a single block, then put a pointer to it into the ROM. We already add the pointer with this: IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size |cut -d' ' -f1) but now it will need to go in a separate place in the ROM. I suggest immediately above the device tree. See the write_uboot() which does all sorts of wacky stuff already. Hopefully we can just replace that code with something that creates the blob. > >> If you like I can take a look at adding this feature to ifdtool. >> Regards, Simon
Hi Simon, On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Currently the microcode-tool writes microcode into a data block as >>>> well as the device tree properties which represents the first 48 >>>> bytes in the microcode data. Now we change the tool to only write >>>> the microcode without device tree stuff so that multiple microcode >>>> data blocks can be included in a single property. >>>> >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>> --- >>>> >>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>> >>> I would rather than we use a tool to pack the microcode together (e.g. >>> ifdtool) rather than changing the source data. I realise that the FSP >>> requires this packing, but I quite dislike the approach of making the >>> source files fit the object files. >>> >> >> Could you roughly describe how you want to do using ifdtool? Is the >> microcode source still from dtb? > > Yes I think it can be done by adding an option to generate the > microcode blob (since for non-FSP platforms it is unnecessary). It can > scan the available microcode nodes and pack them into a single block, > then put a pointer to it into the ROM. > This way we have duplicated microcodes in the ROM. One in the device tree and the other one created by ifdtool as a single block. This causes waste of ROM space. > We already add the pointer with this: > > IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size > |cut -d' ' -f1) > > but now it will need to go in a separate place in the ROM. I suggest > immediately above the device tree. > > See the write_uboot() which does all sorts of wacky stuff already. > Hopefully we can just replace that code with something that creates > the blob. > >> >>> If you like I can take a look at adding this feature to ifdtool. >>> > And I also would like to clean up the microcode dtsi files. The properties below: intel,header-version = <1>; intel,update-revision = <0x105>; intel,date-code = <0x7182011>; intel,processor-signature = <0x20661>; intel,checksum = <0x52558795>; intel,loader-revision = <1>; intel,processor-flags = <0x2>; I think we can remove them. It provides duplicated information as the data property. As data property has to be parsed by us anyway, with these additional properties it adds no value. Regards, Bin
Hi Bin, On 10 August 2015 at 20:45, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Currently the microcode-tool writes microcode into a data block as >>>>> well as the device tree properties which represents the first 48 >>>>> bytes in the microcode data. Now we change the tool to only write >>>>> the microcode without device tree stuff so that multiple microcode >>>>> data blocks can be included in a single property. >>>>> >>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>> --- >>>>> >>>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>>> >>>> I would rather than we use a tool to pack the microcode together (e.g. >>>> ifdtool) rather than changing the source data. I realise that the FSP >>>> requires this packing, but I quite dislike the approach of making the >>>> source files fit the object files. >>>> >>> >>> Could you roughly describe how you want to do using ifdtool? Is the >>> microcode source still from dtb? >> >> Yes I think it can be done by adding an option to generate the >> microcode blob (since for non-FSP platforms it is unnecessary). It can >> scan the available microcode nodes and pack them into a single block, >> then put a pointer to it into the ROM. >> > > This way we have duplicated microcodes in the ROM. One in the device > tree and the other one created by ifdtool as a single block. This > causes waste of ROM space. > >> We already add the pointer with this: >> >> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size >> |cut -d' ' -f1) >> >> but now it will need to go in a separate place in the ROM. I suggest >> immediately above the device tree. >> >> See the write_uboot() which does all sorts of wacky stuff already. >> Hopefully we can just replace that code with something that creates >> the blob. >> >>> >>>> If you like I can take a look at adding this feature to ifdtool. >>>> >> > > And I also would like to clean up the microcode dtsi files. The > properties below: > > intel,header-version = <1>; > intel,update-revision = <0x105>; > intel,date-code = <0x7182011>; > intel,processor-signature = <0x20661>; > intel,checksum = <0x52558795>; > intel,loader-revision = <1>; > intel,processor-flags = <0x2>; > > I think we can remove them. It provides duplicated information as the > data property. As data property has to be parsed by us anyway, with > these additional properties it adds no value. Don't you think these are much easier to read than the binary header? Also we could avoid repeating the data in the data property if we wanted to, since ifdtool can fix this. I only added it because I was unable to get microcode setup to work later on in U-Boot. Regards, Simon
Hi Simon, On Tue, Aug 11, 2015 at 10:51 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 10 August 2015 at 20:45, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> Hi Bin, >>>>> >>>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Currently the microcode-tool writes microcode into a data block as >>>>>> well as the device tree properties which represents the first 48 >>>>>> bytes in the microcode data. Now we change the tool to only write >>>>>> the microcode without device tree stuff so that multiple microcode >>>>>> data blocks can be included in a single property. >>>>>> >>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>> --- >>>>>> >>>>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>>>> >>>>> I would rather than we use a tool to pack the microcode together (e.g. >>>>> ifdtool) rather than changing the source data. I realise that the FSP >>>>> requires this packing, but I quite dislike the approach of making the >>>>> source files fit the object files. >>>>> >>>> >>>> Could you roughly describe how you want to do using ifdtool? Is the >>>> microcode source still from dtb? >>> >>> Yes I think it can be done by adding an option to generate the >>> microcode blob (since for non-FSP platforms it is unnecessary). It can >>> scan the available microcode nodes and pack them into a single block, >>> then put a pointer to it into the ROM. >>> >> >> This way we have duplicated microcodes in the ROM. One in the device >> tree and the other one created by ifdtool as a single block. This >> causes waste of ROM space. >> >>> We already add the pointer with this: >>> >>> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size >>> |cut -d' ' -f1) >>> >>> but now it will need to go in a separate place in the ROM. I suggest >>> immediately above the device tree. >>> >>> See the write_uboot() which does all sorts of wacky stuff already. >>> Hopefully we can just replace that code with something that creates >>> the blob. >>> >>>> >>>>> If you like I can take a look at adding this feature to ifdtool. >>>>> >>> >> >> And I also would like to clean up the microcode dtsi files. The >> properties below: >> >> intel,header-version = <1>; >> intel,update-revision = <0x105>; >> intel,date-code = <0x7182011>; >> intel,processor-signature = <0x20661>; >> intel,checksum = <0x52558795>; >> intel,loader-revision = <1>; >> intel,processor-flags = <0x2>; >> >> I think we can remove them. It provides duplicated information as the >> data property. As data property has to be parsed by us anyway, with >> these additional properties it adds no value. > > Don't you think these are much easier to read than the binary header? > Yes, but I still think duplication is not good. And if we want to do duplication, we should add two more properties (intel,microcode-data-size and intel,microcode-total-size) to fully describe the first 48 bytes header. Right now the description is somewhere in between. > Also we could avoid repeating the data in the data property if we > wanted to, since ifdtool can fix this. I only added it because I was > unable to get microcode setup to work later on in U-Boot. > If we use ifdtool, the header data cannot be avoided as FSP needs the header. It looks like we may add some special handling in ifdtool when generating FSP based ROM. These microcode blocks can consume lots of ROM space if we keep two copies. We can have ifdtool to remove the microcode nodes in the dtb to avoid duplication, but I don't like that way. I think putting microcode in the device tree is good for future reference from U-Boot. Regards, Bin
Hi Bin, On 10 August 2015 at 21:05, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 11, 2015 at 10:51 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 10 August 2015 at 20:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Currently the microcode-tool writes microcode into a data block as >>>>>>> well as the device tree properties which represents the first 48 >>>>>>> bytes in the microcode data. Now we change the tool to only write >>>>>>> the microcode without device tree stuff so that multiple microcode >>>>>>> data blocks can be included in a single property. >>>>>>> >>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>> --- >>>>>>> >>>>>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>>>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>>>>> >>>>>> I would rather than we use a tool to pack the microcode together (e.g. >>>>>> ifdtool) rather than changing the source data. I realise that the FSP >>>>>> requires this packing, but I quite dislike the approach of making the >>>>>> source files fit the object files. >>>>>> >>>>> >>>>> Could you roughly describe how you want to do using ifdtool? Is the >>>>> microcode source still from dtb? >>>> >>>> Yes I think it can be done by adding an option to generate the >>>> microcode blob (since for non-FSP platforms it is unnecessary). It can >>>> scan the available microcode nodes and pack them into a single block, >>>> then put a pointer to it into the ROM. >>>> >>> >>> This way we have duplicated microcodes in the ROM. One in the device >>> tree and the other one created by ifdtool as a single block. This >>> causes waste of ROM space. >>> >>>> We already add the pointer with this: >>>> >>>> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size >>>> |cut -d' ' -f1) >>>> >>>> but now it will need to go in a separate place in the ROM. I suggest >>>> immediately above the device tree. >>>> >>>> See the write_uboot() which does all sorts of wacky stuff already. >>>> Hopefully we can just replace that code with something that creates >>>> the blob. >>>> >>>>> >>>>>> If you like I can take a look at adding this feature to ifdtool. >>>>>> >>>> >>> >>> And I also would like to clean up the microcode dtsi files. The >>> properties below: >>> >>> intel,header-version = <1>; >>> intel,update-revision = <0x105>; >>> intel,date-code = <0x7182011>; >>> intel,processor-signature = <0x20661>; >>> intel,checksum = <0x52558795>; >>> intel,loader-revision = <1>; >>> intel,processor-flags = <0x2>; >>> >>> I think we can remove them. It provides duplicated information as the >>> data property. As data property has to be parsed by us anyway, with >>> these additional properties it adds no value. >> >> Don't you think these are much easier to read than the binary header? >> > > Yes, but I still think duplication is not good. And if we want to do > duplication, we should add two more properties > (intel,microcode-data-size and intel,microcode-total-size) to fully > describe the first 48 bytes header. Right now the description is > somewhere in between. Well at least one of those could be found by looking at the length of the 'data' property. But yes I suppose we may as well include everything. > >> Also we could avoid repeating the data in the data property if we >> wanted to, since ifdtool can fix this. I only added it because I was >> unable to get microcode setup to work later on in U-Boot. >> > > If we use ifdtool, the header data cannot be avoided as FSP needs the > header. It looks like we may add some special handling in ifdtool when > generating FSP based ROM. These microcode blocks can consume lots of > ROM space if we keep two copies. We can have ifdtool to remove the > microcode nodes in the dtb to avoid duplication, but I don't like that > way. I think putting microcode in the device tree is good for future > reference from U-Boot. Yes and hopefully one day Intel will make microcode update from FSP easier and we can go back to using device tree normally. I agree two copies is a waste but it doesn't seem like a problem for now. I agree that dropping the microcode from the fdt isn't worth it, but if we want to later, then fdtgrep can do it pretty easily. Mostly I just think that the fdt microcode files are much more friendly and readable than the binary dumps. Regards, Simon
Hi Simon, On Tue, Aug 11, 2015 at 11:15 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 10 August 2015 at 21:05, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Tue, Aug 11, 2015 at 10:51 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 10 August 2015 at 20:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: >>>>> Hi Bin, >>>>> >>>>> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> Currently the microcode-tool writes microcode into a data block as >>>>>>>> well as the device tree properties which represents the first 48 >>>>>>>> bytes in the microcode data. Now we change the tool to only write >>>>>>>> the microcode without device tree stuff so that multiple microcode >>>>>>>> data blocks can be included in a single property. >>>>>>>> >>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>> --- >>>>>>>> >>>>>>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>>>>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>>>>>> >>>>>>> I would rather than we use a tool to pack the microcode together (e.g. >>>>>>> ifdtool) rather than changing the source data. I realise that the FSP >>>>>>> requires this packing, but I quite dislike the approach of making the >>>>>>> source files fit the object files. >>>>>>> >>>>>> >>>>>> Could you roughly describe how you want to do using ifdtool? Is the >>>>>> microcode source still from dtb? >>>>> >>>>> Yes I think it can be done by adding an option to generate the >>>>> microcode blob (since for non-FSP platforms it is unnecessary). It can >>>>> scan the available microcode nodes and pack them into a single block, >>>>> then put a pointer to it into the ROM. >>>>> >>>> >>>> This way we have duplicated microcodes in the ROM. One in the device >>>> tree and the other one created by ifdtool as a single block. This >>>> causes waste of ROM space. >>>> >>>>> We already add the pointer with this: >>>>> >>>>> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size >>>>> |cut -d' ' -f1) >>>>> >>>>> but now it will need to go in a separate place in the ROM. I suggest >>>>> immediately above the device tree. >>>>> >>>>> See the write_uboot() which does all sorts of wacky stuff already. >>>>> Hopefully we can just replace that code with something that creates >>>>> the blob. >>>>> >>>>>> >>>>>>> If you like I can take a look at adding this feature to ifdtool. >>>>>>> >>>>> >>>> >>>> And I also would like to clean up the microcode dtsi files. The >>>> properties below: >>>> >>>> intel,header-version = <1>; >>>> intel,update-revision = <0x105>; >>>> intel,date-code = <0x7182011>; >>>> intel,processor-signature = <0x20661>; >>>> intel,checksum = <0x52558795>; >>>> intel,loader-revision = <1>; >>>> intel,processor-flags = <0x2>; >>>> >>>> I think we can remove them. It provides duplicated information as the >>>> data property. As data property has to be parsed by us anyway, with >>>> these additional properties it adds no value. >>> >>> Don't you think these are much easier to read than the binary header? >>> >> >> Yes, but I still think duplication is not good. And if we want to do >> duplication, we should add two more properties >> (intel,microcode-data-size and intel,microcode-total-size) to fully >> describe the first 48 bytes header. Right now the description is >> somewhere in between. > > Well at least one of those could be found by looking at the length of > the 'data' property. But yes I suppose we may as well include > everything. > >> >>> Also we could avoid repeating the data in the data property if we >>> wanted to, since ifdtool can fix this. I only added it because I was >>> unable to get microcode setup to work later on in U-Boot. >>> >> >> If we use ifdtool, the header data cannot be avoided as FSP needs the >> header. It looks like we may add some special handling in ifdtool when >> generating FSP based ROM. These microcode blocks can consume lots of >> ROM space if we keep two copies. We can have ifdtool to remove the >> microcode nodes in the dtb to avoid duplication, but I don't like that >> way. I think putting microcode in the device tree is good for future >> reference from U-Boot. > > Yes and hopefully one day Intel will make microcode update from FSP > easier and we can go back to using device tree normally. As I said before, this is a kind of limitation exposed by certain processors. On some processors, without first applying microcode updates, the CAR just won't work. Thus it is required to load microcode update before initializing CAR and that's why Intel FSP implementation chose doing this way. I believe the correct way should be that Intel stops manufacturing malfunctional processors like this :-) > I agree two copies is a waste but it doesn't seem like a problem for > now. I agree that dropping the microcode from the fdt isn't worth it, > but if we want to later, then fdtgrep can do it pretty easily. I am afraid that, keeping two copies with multiple microcode blobs will make U-Boot does not fit the 1MB ROM size. Yes, it is not a problem since we have large SPI flash, but keeping U-Boot ROM small is one of our goals, isn't it? > Mostly I just think that the fdt microcode files are much more > friendly and readable than the binary dumps. > I still think my series seems to be the easiest way to do this. But if you want to use ifdtool, I am OK with that too. Regards, Bin
Hi Bin, On 10 August 2015 at 21:45, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 11, 2015 at 11:15 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 10 August 2015 at 21:05, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Tue, Aug 11, 2015 at 10:51 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 10 August 2015 at 20:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg@chromium.org> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> Currently the microcode-tool writes microcode into a data block as >>>>>>>>> well as the device tree properties which represents the first 48 >>>>>>>>> bytes in the microcode data. Now we change the tool to only write >>>>>>>>> the microcode without device tree stuff so that multiple microcode >>>>>>>>> data blocks can be included in a single property. >>>>>>>>> >>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> tools/microcode-tool.py | 23 ++++++++++++----------- >>>>>>>>> 1 file changed, 12 insertions(+), 11 deletions(-) >>>>>>>> >>>>>>>> I would rather than we use a tool to pack the microcode together (e.g. >>>>>>>> ifdtool) rather than changing the source data. I realise that the FSP >>>>>>>> requires this packing, but I quite dislike the approach of making the >>>>>>>> source files fit the object files. >>>>>>>> >>>>>>> >>>>>>> Could you roughly describe how you want to do using ifdtool? Is the >>>>>>> microcode source still from dtb? >>>>>> >>>>>> Yes I think it can be done by adding an option to generate the >>>>>> microcode blob (since for non-FSP platforms it is unnecessary). It can >>>>>> scan the available microcode nodes and pack them into a single block, >>>>>> then put a pointer to it into the ROM. >>>>>> >>>>> >>>>> This way we have duplicated microcodes in the ROM. One in the device >>>>> tree and the other one created by ifdtool as a single block. This >>>>> causes waste of ROM space. >>>>> >>>>>> We already add the pointer with this: >>>>>> >>>>>> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size >>>>>> |cut -d' ' -f1) >>>>>> >>>>>> but now it will need to go in a separate place in the ROM. I suggest >>>>>> immediately above the device tree. >>>>>> >>>>>> See the write_uboot() which does all sorts of wacky stuff already. >>>>>> Hopefully we can just replace that code with something that creates >>>>>> the blob. >>>>>> >>>>>>> >>>>>>>> If you like I can take a look at adding this feature to ifdtool. >>>>>>>> >>>>>> >>>>> >>>>> And I also would like to clean up the microcode dtsi files. The >>>>> properties below: >>>>> >>>>> intel,header-version = <1>; >>>>> intel,update-revision = <0x105>; >>>>> intel,date-code = <0x7182011>; >>>>> intel,processor-signature = <0x20661>; >>>>> intel,checksum = <0x52558795>; >>>>> intel,loader-revision = <1>; >>>>> intel,processor-flags = <0x2>; >>>>> >>>>> I think we can remove them. It provides duplicated information as the >>>>> data property. As data property has to be parsed by us anyway, with >>>>> these additional properties it adds no value. >>>> >>>> Don't you think these are much easier to read than the binary header? >>>> >>> >>> Yes, but I still think duplication is not good. And if we want to do >>> duplication, we should add two more properties >>> (intel,microcode-data-size and intel,microcode-total-size) to fully >>> describe the first 48 bytes header. Right now the description is >>> somewhere in between. >> >> Well at least one of those could be found by looking at the length of >> the 'data' property. But yes I suppose we may as well include >> everything. >> >>> >>>> Also we could avoid repeating the data in the data property if we >>>> wanted to, since ifdtool can fix this. I only added it because I was >>>> unable to get microcode setup to work later on in U-Boot. >>>> >>> >>> If we use ifdtool, the header data cannot be avoided as FSP needs the >>> header. It looks like we may add some special handling in ifdtool when >>> generating FSP based ROM. These microcode blocks can consume lots of >>> ROM space if we keep two copies. We can have ifdtool to remove the >>> microcode nodes in the dtb to avoid duplication, but I don't like that >>> way. I think putting microcode in the device tree is good for future >>> reference from U-Boot. >> >> Yes and hopefully one day Intel will make microcode update from FSP >> easier and we can go back to using device tree normally. > > As I said before, this is a kind of limitation exposed by certain > processors. On some processors, without first applying microcode > updates, the CAR just won't work. Thus it is required to load > microcode update before initializing CAR and that's why Intel FSP > implementation chose doing this way. I believe the correct way should > be that Intel stops manufacturing malfunctional processors like this > :-) Sure, and perhaps one day we will drop it. But honestly I really dislike designing the system to the lowest common denominator just because the chip validation failed somewhere. > >> I agree two copies is a waste but it doesn't seem like a problem for >> now. I agree that dropping the microcode from the fdt isn't worth it, >> but if we want to later, then fdtgrep can do it pretty easily. > > I am afraid that, keeping two copies with multiple microcode blobs > will make U-Boot does not fit the 1MB ROM size. Yes, it is not a > problem since we have large SPI flash, but keeping U-Boot ROM small is > one of our goals, isn't it? Yes, and we can have that if we want it. My point is that it doesn't matter for now - it's a problem to solve when needed. > >> Mostly I just think that the fdt microcode files are much more >> friendly and readable than the binary dumps. >> > > I still think my series seems to be the easiest way to do this. But if > you want to use ifdtool, I am OK with that too. I agree it is easier, and with most supported boards using FSPs it seems simpler, but I would much rather go with the ifdtool approach. If we are adding hacks to ifdtool let's make it deal with the original source and do all the transformations, not just some. Regards, Simon
On Fri, Aug 07, 2015 at 02:28:43AM -0700, Bin Meng wrote: > Currently the microcode-tool writes microcode into a data block as > well as the device tree properties which represents the first 48 > bytes in the microcode data. Now we change the tool to only write > the microcode without device tree stuff so that multiple microcode > data blocks can be included in a single property. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> Tested-by: Tom Rini <trini@konsulko.com>
Hi Simon, On Thu, Aug 13, 2015 at 9:56 PM, Tom Rini <trini@konsulko.com> wrote: > On Fri, Aug 07, 2015 at 02:28:43AM -0700, Bin Meng wrote: > >> Currently the microcode-tool writes microcode into a data block as >> well as the device tree properties which represents the first 48 >> bytes in the microcode data. Now we change the tool to only write >> the microcode without device tree stuff so that multiple microcode >> data blocks can be included in a single property. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > Tested-by: Tom Rini <trini@konsulko.com> > > -- Given there are people (Tom/Andrew/Saket) whose board needs the newer stepping microcode, and Tom has tested this series, can we apply this series for now and leave the ifdtool update for future change? Regards, Bin
Hi Bin, On 13 August 2015 at 08:06, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Thu, Aug 13, 2015 at 9:56 PM, Tom Rini <trini@konsulko.com> wrote: >> On Fri, Aug 07, 2015 at 02:28:43AM -0700, Bin Meng wrote: >> >>> Currently the microcode-tool writes microcode into a data block as >>> well as the device tree properties which represents the first 48 >>> bytes in the microcode data. Now we change the tool to only write >>> the microcode without device tree stuff so that multiple microcode >>> data blocks can be included in a single property. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> Tested-by: Tom Rini <trini@konsulko.com> >> >> -- > > Given there are people (Tom/Andrew/Saket) whose board needs the newer > stepping microcode, and Tom has tested this series, can we apply this > series for now and leave the ifdtool update for future change? I would rather sort it out now - I can try to prepare a patch. Regards, Simon
diff --git a/tools/microcode-tool.py b/tools/microcode-tool.py index 71c2e91..5522ffc 100755 --- a/tools/microcode-tool.py +++ b/tools/microcode-tool.py @@ -173,20 +173,21 @@ def CreateFile(date, license_text, mcodes, outfile): * node. * * Date: %s + * + * Device tree properties for this microcode: + * + * compatible = "intel,microcode"; + * intel,header-version = <%d>; + * intel,update-revision = <%#x>; + * intel,date-code = <%#x>; + * intel,processor-signature = <%#x>; + * intel,checksum = <%#x>; + * intel,loader-revision = <%d>; + * intel,processor-flags = <%#x>; */ -compatible = "intel,microcode"; -intel,header-version = <%d>; -intel,update-revision = <%#x>; -intel,date-code = <%#x>; -intel,processor-signature = <%#x>; -intel,checksum = <%#x>; -intel,loader-revision = <%d>; -intel,processor-flags = <%#x>; - /* The first 48-bytes are the public header which repeats the above data */ -data = <%s -\t>;''' +%s''' words = '' add_comments = len(mcodes) > 1 for mcode in mcodes:
Currently the microcode-tool writes microcode into a data block as well as the device tree properties which represents the first 48 bytes in the microcode data. Now we change the tool to only write the microcode without device tree stuff so that multiple microcode data blocks can be included in a single property. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- tools/microcode-tool.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)