diff mbox series

[edk2,edk2-platforms,1/1] Silicon/SynQuacer: add optional OP-TEE DT node

Message ID 1533034935-21530-2-git-send-email-sumit.garg@linaro.org
State Superseded
Headers show
Series [edk2,edk2-platforms,1/1] Silicon/SynQuacer: add optional OP-TEE DT node | expand

Commit Message

Sumit Garg July 31, 2018, 11:02 a.m. UTC 
OP-TEE is optional on Developerbox controlled via SCP firmware. To check
if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

---
 Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +
 .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++
 .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++
 .../SynQuacerDtbLoaderLib.inf                      |  2 ++
 4 files changed, 38 insertions(+)

-- 
2.7.4

Comments

Ard Biesheuvel July 31, 2018, 12:58 p.m. UTC | #1 
On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:
> OP-TEE is optional on Developerbox controlled via SCP firmware. To check

> if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> ---

>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

>  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

>  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

>  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

>  4 files changed, 38 insertions(+)

>

> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> index fc498eb65217..4ff5df978e8e 100644

> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> @@ -76,6 +76,7 @@ [LibraryClasses.common]

>    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

>    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

>    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

> +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

>

>    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

>    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> index 37d642e4b237..d109a5742793 100644

> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> @@ -574,6 +574,13 @@

>          #address-cells = <1>;

>          #size-cells = <0>;

>      };

> +

> +    firmware {

> +        optee {

> +            compatible = "linaro,optee-tz";

> +            method = "smc";

> +        };

> +    };

>  };

>

>  #include "SynQuacerCaches.dtsi"

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

> index 897d06743708..b16e384262b0 100644

> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> @@ -19,6 +19,7 @@

>  #include <Library/DebugLib.h>

>  #include <Library/DxeServicesLib.h>

>  #include <Library/MemoryAllocationLib.h>

> +#include <Library/OpteeLib.h>

>  #include <Platform/VarStore.h>

>

>  // add enough space for three instances of 'status = "disabled"'

> @@ -47,6 +48,29 @@ DisableDtNode (

>    }

>  }

>

> +STATIC

> +VOID

> +DeleteDtNode (

> +  IN  VOID                        *Dtb,

> +  IN  CONST CHAR8                 *NodePath

> +  )

> +{

> +  INT32                           Node;

> +  INT32                           Rc;

> +

> +  Node = fdt_path_offset (Dtb, NodePath);

> +  if (Node < 0) {

> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

> +      __FUNCTION__, NodePath, fdt_strerror (Node)));

> +    return;

> +  }

> +  Rc = fdt_del_node (Dtb, Node);

> +  if (Rc < 0) {

> +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

> +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

> +  }

> +}

> +

>  /**

>    Return a pool allocated copy of the DTB image that is appropriate for

>    booting the current platform via DT.

> @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

>      DisableDtNode (CopyDtb, "/sdhci@52300000");

>    }

>

> +  if (!IsOpteePresent()) {

> +    DeleteDtNode (CopyDtb, "/firmware/optee");

> +  }

> +


Would it be possible to disable the DT node instead?

>    *Dtb = CopyDtb;

>    *DtbSize = CopyDtbSize;

>

> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> index 548d62fd5c0a..fd21f7c376ce 100644

> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> @@ -24,6 +24,7 @@ [Sources]

>    SynQuacerDtbLoaderLib.c

>

>  [Packages]

> +  ArmPkg/ArmPkg.dec

>    MdePkg/MdePkg.dec

>    EmbeddedPkg/EmbeddedPkg.dec

>    Silicon/Socionext/SynQuacer/SynQuacer.dec

> @@ -34,6 +35,7 @@ [LibraryClasses]

>    DxeServicesLib

>    FdtLib

>    MemoryAllocationLib

> +  OpteeLib

>

>  [Pcd]

>    gSynQuacerTokenSpaceGuid.PcdPcieEnableMask

> --

> 2.7.4

>
Sumit Garg July 31, 2018, 1:19 p.m. UTC | #2 
On Tue, 31 Jul 2018 at 18:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:

> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check

> > if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > ---

> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

> >  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

> >  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

> >  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

> >  4 files changed, 38 insertions(+)

> >

> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> > index fc498eb65217..4ff5df978e8e 100644

> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> > @@ -76,6 +76,7 @@ [LibraryClasses.common]

> >    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

> >    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

> >    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

> > +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

> >

> >    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

> >    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> > index 37d642e4b237..d109a5742793 100644

> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> > @@ -574,6 +574,13 @@

> >          #address-cells = <1>;

> >          #size-cells = <0>;

> >      };

> > +

> > +    firmware {

> > +        optee {

> > +            compatible = "linaro,optee-tz";

> > +            method = "smc";

> > +        };

> > +    };

> >  };

> >

> >  #include "SynQuacerCaches.dtsi"

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

> > index 897d06743708..b16e384262b0 100644

> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> > @@ -19,6 +19,7 @@

> >  #include <Library/DebugLib.h>

> >  #include <Library/DxeServicesLib.h>

> >  #include <Library/MemoryAllocationLib.h>

> > +#include <Library/OpteeLib.h>

> >  #include <Platform/VarStore.h>

> >

> >  // add enough space for three instances of 'status = "disabled"'

> > @@ -47,6 +48,29 @@ DisableDtNode (

> >    }

> >  }

> >

> > +STATIC

> > +VOID

> > +DeleteDtNode (

> > +  IN  VOID                        *Dtb,

> > +  IN  CONST CHAR8                 *NodePath

> > +  )

> > +{

> > +  INT32                           Node;

> > +  INT32                           Rc;

> > +

> > +  Node = fdt_path_offset (Dtb, NodePath);

> > +  if (Node < 0) {

> > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

> > +      __FUNCTION__, NodePath, fdt_strerror (Node)));

> > +    return;

> > +  }

> > +  Rc = fdt_del_node (Dtb, Node);

> > +  if (Rc < 0) {

> > +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

> > +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

> > +  }

> > +}

> > +

> >  /**

> >    Return a pool allocated copy of the DTB image that is appropriate for

> >    booting the current platform via DT.

> > @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

> >      DisableDtNode (CopyDtb, "/sdhci@52300000");

> >    }

> >

> > +  if (!IsOpteePresent()) {

> > +    DeleteDtNode (CopyDtb, "/firmware/optee");

> > +  }

> > +

>

> Would it be possible to disable the DT node instead?

>


I did tried to explore it. But OP-TEE driver in Linux doesn't seem to
parse "status" field.

-Sumit

> >    *Dtb = CopyDtb;

> >    *DtbSize = CopyDtbSize;

> >

> > diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> > index 548d62fd5c0a..fd21f7c376ce 100644

> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf

> > @@ -24,6 +24,7 @@ [Sources]

> >    SynQuacerDtbLoaderLib.c

> >

> >  [Packages]

> > +  ArmPkg/ArmPkg.dec

> >    MdePkg/MdePkg.dec

> >    EmbeddedPkg/EmbeddedPkg.dec

> >    Silicon/Socionext/SynQuacer/SynQuacer.dec

> > @@ -34,6 +35,7 @@ [LibraryClasses]

> >    DxeServicesLib

> >    FdtLib

> >    MemoryAllocationLib

> > +  OpteeLib

> >

> >  [Pcd]

> >    gSynQuacerTokenSpaceGuid.PcdPcieEnableMask

> > --

> > 2.7.4

> >
Ard Biesheuvel July 31, 2018, 1:22 p.m. UTC | #3 
(+ Jens)

On 31 July 2018 at 15:19, Sumit Garg <sumit.garg@linaro.org> wrote:
> On Tue, 31 Jul 2018 at 18:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>

>> On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:

>> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check

>> > if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

>> >

>> > Contributed-under: TianoCore Contribution Agreement 1.1

>> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

>> > ---

>> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

>> >  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

>> >  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

>> >  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

>> >  4 files changed, 38 insertions(+)

>> >

>> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> > index fc498eb65217..4ff5df978e8e 100644

>> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> > @@ -76,6 +76,7 @@ [LibraryClasses.common]

>> >    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

>> >    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

>> >    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

>> > +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

>> >

>> >    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

>> >    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

>> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> > index 37d642e4b237..d109a5742793 100644

>> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> > @@ -574,6 +574,13 @@

>> >          #address-cells = <1>;

>> >          #size-cells = <0>;

>> >      };

>> > +

>> > +    firmware {

>> > +        optee {

>> > +            compatible = "linaro,optee-tz";

>> > +            method = "smc";

>> > +        };

>> > +    };

>> >  };

>> >

>> >  #include "SynQuacerCaches.dtsi"

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

>> > index 897d06743708..b16e384262b0 100644

>> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>> > @@ -19,6 +19,7 @@

>> >  #include <Library/DebugLib.h>

>> >  #include <Library/DxeServicesLib.h>

>> >  #include <Library/MemoryAllocationLib.h>

>> > +#include <Library/OpteeLib.h>

>> >  #include <Platform/VarStore.h>

>> >

>> >  // add enough space for three instances of 'status = "disabled"'

>> > @@ -47,6 +48,29 @@ DisableDtNode (

>> >    }

>> >  }

>> >

>> > +STATIC

>> > +VOID

>> > +DeleteDtNode (

>> > +  IN  VOID                        *Dtb,

>> > +  IN  CONST CHAR8                 *NodePath

>> > +  )

>> > +{

>> > +  INT32                           Node;

>> > +  INT32                           Rc;

>> > +

>> > +  Node = fdt_path_offset (Dtb, NodePath);

>> > +  if (Node < 0) {

>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

>> > +      __FUNCTION__, NodePath, fdt_strerror (Node)));

>> > +    return;

>> > +  }

>> > +  Rc = fdt_del_node (Dtb, Node);

>> > +  if (Rc < 0) {

>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

>> > +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

>> > +  }

>> > +}

>> > +

>> >  /**

>> >    Return a pool allocated copy of the DTB image that is appropriate for

>> >    booting the current platform via DT.

>> > @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

>> >      DisableDtNode (CopyDtb, "/sdhci@52300000");

>> >    }

>> >

>> > +  if (!IsOpteePresent()) {

>> > +    DeleteDtNode (CopyDtb, "/firmware/optee");

>> > +  }

>> > +

>>

>> Would it be possible to disable the DT node instead?

>>

>

> I did tried to explore it. But OP-TEE driver in Linux doesn't seem to

> parse "status" field.

>


That is probably an oversight: I would not expect any DT driver to
touch its device if the node's status is set to 'disabled'.

Jens?
Ard Biesheuvel Aug. 1, 2018, 9:54 a.m. UTC | #4 
On 31 July 2018 at 15:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> (+ Jens)

>

> On 31 July 2018 at 15:19, Sumit Garg <sumit.garg@linaro.org> wrote:

>> On Tue, 31 Jul 2018 at 18:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>>

>>> On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:

>>> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check

>>> > if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

>>> >

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

>>> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

>>> > ---

>>> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

>>> >  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

>>> >  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

>>> >  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

>>> >  4 files changed, 38 insertions(+)

>>> >

>>> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>>> > index fc498eb65217..4ff5df978e8e 100644

>>> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>>> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>>> > @@ -76,6 +76,7 @@ [LibraryClasses.common]

>>> >    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

>>> >    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

>>> >    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

>>> > +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

>>> >

>>> >    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

>>> >    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

>>> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>>> > index 37d642e4b237..d109a5742793 100644

>>> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>>> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>>> > @@ -574,6 +574,13 @@

>>> >          #address-cells = <1>;

>>> >          #size-cells = <0>;

>>> >      };

>>> > +

>>> > +    firmware {

>>> > +        optee {

>>> > +            compatible = "linaro,optee-tz";

>>> > +            method = "smc";

>>> > +        };

>>> > +    };

>>> >  };

>>> >

>>> >  #include "SynQuacerCaches.dtsi"

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

>>> > index 897d06743708..b16e384262b0 100644

>>> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>>> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>>> > @@ -19,6 +19,7 @@

>>> >  #include <Library/DebugLib.h>

>>> >  #include <Library/DxeServicesLib.h>

>>> >  #include <Library/MemoryAllocationLib.h>

>>> > +#include <Library/OpteeLib.h>

>>> >  #include <Platform/VarStore.h>

>>> >

>>> >  // add enough space for three instances of 'status = "disabled"'

>>> > @@ -47,6 +48,29 @@ DisableDtNode (

>>> >    }

>>> >  }

>>> >

>>> > +STATIC

>>> > +VOID

>>> > +DeleteDtNode (

>>> > +  IN  VOID                        *Dtb,

>>> > +  IN  CONST CHAR8                 *NodePath

>>> > +  )

>>> > +{

>>> > +  INT32                           Node;

>>> > +  INT32                           Rc;

>>> > +

>>> > +  Node = fdt_path_offset (Dtb, NodePath);

>>> > +  if (Node < 0) {

>>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

>>> > +      __FUNCTION__, NodePath, fdt_strerror (Node)));

>>> > +    return;

>>> > +  }

>>> > +  Rc = fdt_del_node (Dtb, Node);

>>> > +  if (Rc < 0) {

>>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

>>> > +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

>>> > +  }

>>> > +}

>>> > +

>>> >  /**

>>> >    Return a pool allocated copy of the DTB image that is appropriate for

>>> >    booting the current platform via DT.

>>> > @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

>>> >      DisableDtNode (CopyDtb, "/sdhci@52300000");

>>> >    }

>>> >

>>> > +  if (!IsOpteePresent()) {

>>> > +    DeleteDtNode (CopyDtb, "/firmware/optee");

>>> > +  }

>>> > +

>>>

>>> Would it be possible to disable the DT node instead?

>>>

>>

>> I did tried to explore it. But OP-TEE driver in Linux doesn't seem to

>> parse "status" field.

>>

>

> That is probably an oversight: I would not expect any DT driver to

> touch its device if the node's status is set to 'disabled'.

>

> Jens?


OK, so I have sent out a patch for the linux optee driver

In the meantime, could we change this patch so the DT file has 'status
= "disabled"' by default, and we enable the node by updating the value
to 'okay' ?

Bonus points for a preparatory patch that does the same for the PCIe
and eMMC nodes, so we end up with a single approach for all (and no
need for DTB_PADDING)

Thanks,
Ard.
Sumit Garg Aug. 1, 2018, 11:13 a.m. UTC | #5 
On Wed, 1 Aug 2018 at 15:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On 31 July 2018 at 15:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > (+ Jens)

> >

> > On 31 July 2018 at 15:19, Sumit Garg <sumit.garg@linaro.org> wrote:

> >> On Tue, 31 Jul 2018 at 18:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >>>

> >>> On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:

> >>> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check

> >>> > if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

> >>> >

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

> >>> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> >>> > ---

> >>> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

> >>> >  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

> >>> >  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

> >>> >  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

> >>> >  4 files changed, 38 insertions(+)

> >>> >

> >>> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> >>> > index fc498eb65217..4ff5df978e8e 100644

> >>> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> >>> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

> >>> > @@ -76,6 +76,7 @@ [LibraryClasses.common]

> >>> >    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

> >>> >    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

> >>> >    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

> >>> > +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

> >>> >

> >>> >    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

> >>> >    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

> >>> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >>> > index 37d642e4b237..d109a5742793 100644

> >>> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >>> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >>> > @@ -574,6 +574,13 @@

> >>> >          #address-cells = <1>;

> >>> >          #size-cells = <0>;

> >>> >      };

> >>> > +

> >>> > +    firmware {

> >>> > +        optee {

> >>> > +            compatible = "linaro,optee-tz";

> >>> > +            method = "smc";

> >>> > +        };

> >>> > +    };

> >>> >  };

> >>> >

> >>> >  #include "SynQuacerCaches.dtsi"

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

> >>> > index 897d06743708..b16e384262b0 100644

> >>> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> >>> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

> >>> > @@ -19,6 +19,7 @@

> >>> >  #include <Library/DebugLib.h>

> >>> >  #include <Library/DxeServicesLib.h>

> >>> >  #include <Library/MemoryAllocationLib.h>

> >>> > +#include <Library/OpteeLib.h>

> >>> >  #include <Platform/VarStore.h>

> >>> >

> >>> >  // add enough space for three instances of 'status = "disabled"'

> >>> > @@ -47,6 +48,29 @@ DisableDtNode (

> >>> >    }

> >>> >  }

> >>> >

> >>> > +STATIC

> >>> > +VOID

> >>> > +DeleteDtNode (

> >>> > +  IN  VOID                        *Dtb,

> >>> > +  IN  CONST CHAR8                 *NodePath

> >>> > +  )

> >>> > +{

> >>> > +  INT32                           Node;

> >>> > +  INT32                           Rc;

> >>> > +

> >>> > +  Node = fdt_path_offset (Dtb, NodePath);

> >>> > +  if (Node < 0) {

> >>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

> >>> > +      __FUNCTION__, NodePath, fdt_strerror (Node)));

> >>> > +    return;

> >>> > +  }

> >>> > +  Rc = fdt_del_node (Dtb, Node);

> >>> > +  if (Rc < 0) {

> >>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

> >>> > +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

> >>> > +  }

> >>> > +}

> >>> > +

> >>> >  /**

> >>> >    Return a pool allocated copy of the DTB image that is appropriate for

> >>> >    booting the current platform via DT.

> >>> > @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

> >>> >      DisableDtNode (CopyDtb, "/sdhci@52300000");

> >>> >    }

> >>> >

> >>> > +  if (!IsOpteePresent()) {

> >>> > +    DeleteDtNode (CopyDtb, "/firmware/optee");

> >>> > +  }

> >>> > +

> >>>

> >>> Would it be possible to disable the DT node instead?

> >>>

> >>

> >> I did tried to explore it. But OP-TEE driver in Linux doesn't seem to

> >> parse "status" field.

> >>

> >

> > That is probably an oversight: I would not expect any DT driver to

> > touch its device if the node's status is set to 'disabled'.

> >

> > Jens?

>

> OK, so I have sent out a patch for the linux optee driver

>

> In the meantime, could we change this patch so the DT file has 'status

> = "disabled"' by default, and we enable the node by updating the value

> to 'okay' ?

>


Sure will follow this approach instead and use your patch to test.

> Bonus points for a preparatory patch that does the same for the PCIe

> and eMMC nodes, so we end up with a single approach for all (and no

> need for DTB_PADDING)

>


I would be happy to create a patch for this. Can you please guide me
regarding the test scenarios for PCIe and eMMC nodes? Please keep in
mind that I am working on Daniel's remote setup.

Regards,
Sumit

> Thanks,

> Ard.
Ard Biesheuvel Aug. 1, 2018, 11:15 a.m. UTC | #6 
On 1 August 2018 at 13:13, Sumit Garg <sumit.garg@linaro.org> wrote:
> On Wed, 1 Aug 2018 at 15:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>

>> On 31 July 2018 at 15:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> > (+ Jens)

>> >

>> > On 31 July 2018 at 15:19, Sumit Garg <sumit.garg@linaro.org> wrote:

>> >> On Tue, 31 Jul 2018 at 18:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> >>>

>> >>> On 31 July 2018 at 13:02, Sumit Garg <sumit.garg@linaro.org> wrote:

>> >>> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check

>> >>> > if we need to delete OP-TEE DT node, we use "IsOpteePresent" OpteeLib api.

>> >>> >

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

>> >>> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

>> >>> > ---

>> >>> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc   |  1 +

>> >>> >  .../Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi  |  7 ++++++

>> >>> >  .../SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c  | 28 ++++++++++++++++++++++

>> >>> >  .../SynQuacerDtbLoaderLib.inf                      |  2 ++

>> >>> >  4 files changed, 38 insertions(+)

>> >>> >

>> >>> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> >>> > index fc498eb65217..4ff5df978e8e 100644

>> >>> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> >>> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc

>> >>> > @@ -76,6 +76,7 @@ [LibraryClasses.common]

>> >>> >    ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf

>> >>> >    ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf

>> >>> >    ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf

>> >>> > +  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf

>> >>> >

>> >>> >    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf

>> >>> >    BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf

>> >>> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >>> > index 37d642e4b237..d109a5742793 100644

>> >>> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >>> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >>> > @@ -574,6 +574,13 @@

>> >>> >          #address-cells = <1>;

>> >>> >          #size-cells = <0>;

>> >>> >      };

>> >>> > +

>> >>> > +    firmware {

>> >>> > +        optee {

>> >>> > +            compatible = "linaro,optee-tz";

>> >>> > +            method = "smc";

>> >>> > +        };

>> >>> > +    };

>> >>> >  };

>> >>> >

>> >>> >  #include "SynQuacerCaches.dtsi"

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

>> >>> > index 897d06743708..b16e384262b0 100644

>> >>> > --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>> >>> > +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c

>> >>> > @@ -19,6 +19,7 @@

>> >>> >  #include <Library/DebugLib.h>

>> >>> >  #include <Library/DxeServicesLib.h>

>> >>> >  #include <Library/MemoryAllocationLib.h>

>> >>> > +#include <Library/OpteeLib.h>

>> >>> >  #include <Platform/VarStore.h>

>> >>> >

>> >>> >  // add enough space for three instances of 'status = "disabled"'

>> >>> > @@ -47,6 +48,29 @@ DisableDtNode (

>> >>> >    }

>> >>> >  }

>> >>> >

>> >>> > +STATIC

>> >>> > +VOID

>> >>> > +DeleteDtNode (

>> >>> > +  IN  VOID                        *Dtb,

>> >>> > +  IN  CONST CHAR8                 *NodePath

>> >>> > +  )

>> >>> > +{

>> >>> > +  INT32                           Node;

>> >>> > +  INT32                           Rc;

>> >>> > +

>> >>> > +  Node = fdt_path_offset (Dtb, NodePath);

>> >>> > +  if (Node < 0) {

>> >>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",

>> >>> > +      __FUNCTION__, NodePath, fdt_strerror (Node)));

>> >>> > +    return;

>> >>> > +  }

>> >>> > +  Rc = fdt_del_node (Dtb, Node);

>> >>> > +  if (Rc < 0) {

>> >>> > +    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",

>> >>> > +      __FUNCTION__, NodePath, fdt_strerror (Rc)));

>> >>> > +  }

>> >>> > +}

>> >>> > +

>> >>> >  /**

>> >>> >    Return a pool allocated copy of the DTB image that is appropriate for

>> >>> >    booting the current platform via DT.

>> >>> > @@ -107,6 +131,10 @@ DtPlatformLoadDtb (

>> >>> >      DisableDtNode (CopyDtb, "/sdhci@52300000");

>> >>> >    }

>> >>> >

>> >>> > +  if (!IsOpteePresent()) {

>> >>> > +    DeleteDtNode (CopyDtb, "/firmware/optee");

>> >>> > +  }

>> >>> > +

>> >>>

>> >>> Would it be possible to disable the DT node instead?

>> >>>

>> >>

>> >> I did tried to explore it. But OP-TEE driver in Linux doesn't seem to

>> >> parse "status" field.

>> >>

>> >

>> > That is probably an oversight: I would not expect any DT driver to

>> > touch its device if the node's status is set to 'disabled'.

>> >

>> > Jens?

>>

>> OK, so I have sent out a patch for the linux optee driver

>>

>> In the meantime, could we change this patch so the DT file has 'status

>> = "disabled"' by default, and we enable the node by updating the value

>> to 'okay' ?

>>

>

> Sure will follow this approach instead and use your patch to test.

>

>> Bonus points for a preparatory patch that does the same for the PCIe

>> and eMMC nodes, so we end up with a single approach for all (and no

>> need for DTB_PADDING)

>>

>

> I would be happy to create a patch for this. Can you please guide me

> regarding the test scenarios for PCIe and eMMC nodes? Please keep in

> mind that I am working on Daniel's remote setup.

>


You can test the eMMC code path by en/disabling the eMMC in the UEFI menus

PCIe can only be tested on the EVB, not on DeveloperBox
diff mbox series

Patch

diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index fc498eb65217..4ff5df978e8e 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -76,6 +76,7 @@  [LibraryClasses.common]
   ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
   ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
   ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
+  OpteeLib|ArmPkg/Library/OpteeLib/OpteeLib.inf
 
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
   BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf
diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
index 37d642e4b237..d109a5742793 100644
--- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
+++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
@@ -574,6 +574,13 @@ 
         #address-cells = <1>;
         #size-cells = <0>;
     };
+
+    firmware {
+        optee {
+            compatible = "linaro,optee-tz";
+            method = "smc";
+        };
+    };
 };
 
 #include "SynQuacerCaches.dtsi"
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
index 897d06743708..b16e384262b0 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
@@ -19,6 +19,7 @@ 
 #include <Library/DebugLib.h>
 #include <Library/DxeServicesLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/OpteeLib.h>
 #include <Platform/VarStore.h>
 
 // add enough space for three instances of 'status = "disabled"'
@@ -47,6 +48,29 @@  DisableDtNode (
   }
 }
 
+STATIC
+VOID
+DeleteDtNode (
+  IN  VOID                        *Dtb,
+  IN  CONST CHAR8                 *NodePath
+  )
+{
+  INT32                           Node;
+  INT32                           Rc;
+
+  Node = fdt_path_offset (Dtb, NodePath);
+  if (Node < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",
+      __FUNCTION__, NodePath, fdt_strerror (Node)));
+    return;
+  }
+  Rc = fdt_del_node (Dtb, Node);
+  if (Rc < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",
+      __FUNCTION__, NodePath, fdt_strerror (Rc)));
+  }
+}
+
 /**
   Return a pool allocated copy of the DTB image that is appropriate for
   booting the current platform via DT.
@@ -107,6 +131,10 @@  DtPlatformLoadDtb (
     DisableDtNode (CopyDtb, "/sdhci@52300000");
   }
 
+  if (!IsOpteePresent()) {
+    DeleteDtNode (CopyDtb, "/firmware/optee");
+  }
+
   *Dtb = CopyDtb;
   *DtbSize = CopyDtbSize;
 
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
index 548d62fd5c0a..fd21f7c376ce 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
@@ -24,6 +24,7 @@  [Sources]
   SynQuacerDtbLoaderLib.c
 
 [Packages]
+  ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
   Silicon/Socionext/SynQuacer/SynQuacer.dec
@@ -34,6 +35,7 @@  [LibraryClasses]
   DxeServicesLib
   FdtLib
   MemoryAllocationLib
+  OpteeLib
 
 [Pcd]
   gSynQuacerTokenSpaceGuid.PcdPcieEnableMask