[edk2,edk2-platforms,2/2] Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV

Message ID 20180607150818.14393-3-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • DeveloperBox: prepare for expanding the capsule payload
Related show

Commit Message

Ard Biesheuvel June 7, 2018, 3:08 p.m.
In order to allow for more flexibility when updating parts of the
firmware via capsule update, expand the description of the code FV
to cover the entire 4 MB region at the base of the NOR flash.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm June 12, 2018, 10:59 p.m. | #1
On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:
> In order to allow for more flexibility when updating parts of the

> firmware via capsule update, expand the description of the code FV

> to cover the entire 4 MB region at the base of the NOR flash.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--

>  1 file changed, 3 insertions(+), 2 deletions(-)

> 

> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> index 816d8ba33f8c..357082c3d903 100644

> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {

>    {

>      // UEFI code region

>      SYNQUACER_SPI_NOR_BASE,                             // device base

> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base

> -    FixedPcdGet32 (PcdFdSize),                          // region size

> +    SYNQUACER_SPI_NOR_BASE,                             // region base

> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -

> +    SYNQUACER_SPI_NOR_BASE,                             // region size


Could you define the size as a macro in Platform/MemoryMap.h?

/
    Leif

>      SIZE_64KB,                                          // block size

>      {

>        0x19c118b0, 0xc423, 0x42be, { 0xb8, 0x0f, 0x70, 0x6f, 0x1f, 0xcb, 0x59, 0x9a }

> -- 

> 2.17.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 13, 2018, 7:19 a.m. | #2
On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:

>> In order to allow for more flexibility when updating parts of the

>> firmware via capsule update, expand the description of the code FV

>> to cover the entire 4 MB region at the base of the NOR flash.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--

>>  1 file changed, 3 insertions(+), 2 deletions(-)

>>

>> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> index 816d8ba33f8c..357082c3d903 100644

>> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {

>>    {

>>      // UEFI code region

>>      SYNQUACER_SPI_NOR_BASE,                             // device base

>> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base

>> -    FixedPcdGet32 (PcdFdSize),                          // region size

>> +    SYNQUACER_SPI_NOR_BASE,                             // region base

>> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -

>> +    SYNQUACER_SPI_NOR_BASE,                             // region size

>

> Could you define the size as a macro in Platform/MemoryMap.h?

>


The memory map currently only contains constant macros. I can add this
expression

FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE

somewhere as a #define but I would prefer it to be elsewhere, given
that it is not a SoC constant set in stone.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 13, 2018, 10 a.m. | #3
On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:

> >> In order to allow for more flexibility when updating parts of the

> >> firmware via capsule update, expand the description of the code FV

> >> to cover the entire 4 MB region at the base of the NOR flash.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.1

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> ---

> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--

> >>  1 file changed, 3 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> index 816d8ba33f8c..357082c3d903 100644

> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {

> >>    {

> >>      // UEFI code region

> >>      SYNQUACER_SPI_NOR_BASE,                             // device base

> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base

> >> -    FixedPcdGet32 (PcdFdSize),                          // region size

> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base

> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -

> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size

> >

> > Could you define the size as a macro in Platform/MemoryMap.h?

> >

> 

> The memory map currently only contains constant macros. I can add this

> expression

> 

> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE

> 

> somewhere as a #define but I would prefer it to be elsewhere, given

> that it is not a SoC constant set in stone.


I'm OK with that, but will just throw in the argument that the fact
that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind
of set in stone. (And doesn't the Fixed bit make it constant?)

Either way, my main interest is in making this struct definition not
break my reading flow, so I'm perfectly happy with the #define
elsewhere.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 13, 2018, noon | #4
On 13 June 2018 at 12:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:

>> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:

>> >> In order to allow for more flexibility when updating parts of the

>> >> firmware via capsule update, expand the description of the code FV

>> >> to cover the entire 4 MB region at the base of the NOR flash.

>> >>

>> >> Contributed-under: TianoCore Contribution Agreement 1.1

>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> >> ---

>> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--

>> >>  1 file changed, 3 insertions(+), 2 deletions(-)

>> >>

>> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> >> index 816d8ba33f8c..357082c3d903 100644

>> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

>> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {

>> >>    {

>> >>      // UEFI code region

>> >>      SYNQUACER_SPI_NOR_BASE,                             // device base

>> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base

>> >> -    FixedPcdGet32 (PcdFdSize),                          // region size

>> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base

>> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -

>> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size

>> >

>> > Could you define the size as a macro in Platform/MemoryMap.h?

>> >

>>

>> The memory map currently only contains constant macros. I can add this

>> expression

>>

>> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE

>>

>> somewhere as a #define but I would prefer it to be elsewhere, given

>> that it is not a SoC constant set in stone.

>

> I'm OK with that, but will just throw in the argument that the fact

> that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind

> of set in stone. (And doesn't the Fixed bit make it constant?)

>


The point is more that MemoryMap.h describes properties of the SoC not
properties of our firmware implementation

But that also means that I should introduce a symbolic constant for
the start of the partition, e.g.,

#define FW_CODE_REGION_BASE    SYNQUACER_SPI_NOR_BASE
#define FW_CODE_REGION_SIZE    (FixedPcdGet32 (PcdFlashNvStorageVariableBase)
                               - SYNQUACER_SPI_NOR_BASE)

but I'd still prefer to keep it local to this file.

> Either way, my main interest is in making this struct definition not

> break my reading flow, so I'm perfectly happy with the #define

> elsewhere.

>

> /

>     Leif

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 13, 2018, 12:28 p.m. | #5
On Wed, Jun 13, 2018 at 02:00:39PM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 12:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Wed, Jun 13, 2018 at 09:19:01AM +0200, Ard Biesheuvel wrote:

> >> On 13 June 2018 at 00:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> >> > On Thu, Jun 07, 2018 at 05:08:18PM +0200, Ard Biesheuvel wrote:

> >> >> In order to allow for more flexibility when updating parts of the

> >> >> firmware via capsule update, expand the description of the code FV

> >> >> to cover the entire 4 MB region at the base of the NOR flash.

> >> >>

> >> >> Contributed-under: TianoCore Contribution Agreement 1.1

> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> >> ---

> >> >>  Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c | 5 +++--

> >> >>  1 file changed, 3 insertions(+), 2 deletions(-)

> >> >>

> >> >> diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> >> index 816d8ba33f8c..357082c3d903 100644

> >> >> --- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> >> +++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c

> >> >> @@ -23,8 +23,9 @@ STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {

> >> >>    {

> >> >>      // UEFI code region

> >> >>      SYNQUACER_SPI_NOR_BASE,                             // device base

> >> >> -    FixedPcdGet64 (PcdFdBaseAddress),                   // region base

> >> >> -    FixedPcdGet32 (PcdFdSize),                          // region size

> >> >> +    SYNQUACER_SPI_NOR_BASE,                             // region base

> >> >> +    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -

> >> >> +    SYNQUACER_SPI_NOR_BASE,                             // region size

> >> >

> >> > Could you define the size as a macro in Platform/MemoryMap.h?

> >> >

> >>

> >> The memory map currently only contains constant macros. I can add this

> >> expression

> >>

> >> FixedPcdGet32 (PcdFlashNvStorageVariableBase) - SYNQUACER_SPI_NOR_BASE

> >>

> >> somewhere as a #define but I would prefer it to be elsewhere, given

> >> that it is not a SoC constant set in stone.

> >

> > I'm OK with that, but will just throw in the argument that the fact

> > that it being a FixedPcdGet32 of PcdFlashNvStorageVariableBase is kind

> > of set in stone. (And doesn't the Fixed bit make it constant?)

> >

> 

> The point is more that MemoryMap.h describes properties of the SoC not

> properties of our firmware implementation

> 

> But that also means that I should introduce a symbolic constant for

> the start of the partition, e.g.,

> 

> #define FW_CODE_REGION_BASE    SYNQUACER_SPI_NOR_BASE

> #define FW_CODE_REGION_SIZE    (FixedPcdGet32 (PcdFlashNvStorageVariableBase)

>                                - SYNQUACER_SPI_NOR_BASE)

> 

> but I'd still prefer to keep it local to this file.


Yeah, that's fine.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
index 816d8ba33f8c..357082c3d903 100644
--- a/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
+++ b/Silicon/Socionext/SynQuacer/Library/NorFlashSynQuacerLib/NorFlashSynQuacer.c
@@ -23,8 +23,9 @@  STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
   {
     // UEFI code region
     SYNQUACER_SPI_NOR_BASE,                             // device base
-    FixedPcdGet64 (PcdFdBaseAddress),                   // region base
-    FixedPcdGet32 (PcdFdSize),                          // region size
+    SYNQUACER_SPI_NOR_BASE,                             // region base
+    FixedPcdGet32 (PcdFlashNvStorageVariableBase) -
+    SYNQUACER_SPI_NOR_BASE,                             // region size
     SIZE_64KB,                                          // block size
     {
       0x19c118b0, 0xc423, 0x42be, { 0xb8, 0x0f, 0x70, 0x6f, 0x1f, 0xcb, 0x59, 0x9a }