Re: [edk2] Uefi runtime property in device tree

[Pankaj Bansal]

Hello Ard,

Thanks for replying

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, December 26, 2018 7:16 PM
> To: Pankaj Bansal 
> Cc: Varun Sethi ; Udit Kumar ; linux-
> efi ; edk2-devel@lists.01.org
> Subject: Re: Uefi runtime property in device tree
> 
> On Wed, 26 Dec 2018 at 11:15, Pankaj Bansal  wrote:
> >
> > Hello Ard et al.
> >
> > I have a query regarding the device tree usage in UEFI.
> > In our UEFI implementation for NXP SOCs, we are using device tree to detect
> Non discoverable platform devices.
> > Based on the device detected in device tree, a device instance is created 
> > and
> the device’s driver binds to that device’s handle (a DXE driver or an UEFI 
> driver).
> > if the device were to be used for runtime service, then we need to allocate 
> > the
> memory for that device instance from runtime pool and set its virtual address
> using EfiConvertPointer.
> > To facilitate this, I wish to add an optional property to the device node 
> > “uefi-
> runtime”.
> > If this property is present in device tree the UEFI firmware will allocate 
> > the
> data from runtime pool for this device.
> > Also firmware will disable/delete the node in device tree before passing 
> > onto
> OS, so that OS doesn’t use the device.
> >
> > I wish to know your thoughts on this. If this doesn’t seem the right way, I 
> > am
> happy to hear your suggestions.
> >
> 
> Hello Pankaj,
> 
> In general, you are free to do whatever you like in the internal 
> implementation
> of your firmware. You can invent your own DT properties, bindings, etc, or 
> even
> invent your own description language altogether as long as you ensure that the
> descriptions don't leak into places where they are visible to the OS.
> 
> However, I do wonder how generic this has to be. Since the DT and the firmware
> will be tightly coupled in any case, what is preventing you from defining the 
> RTC
> and NOR flash devices as DT device paths in the firmware source, and attaching
> to them directly rather than traversing the device tree looking for 
> uefi_runtime
> nodes.

The UEFI driver model that we are following, creates the device instances for 
all the devices
attached to a controller by parsing DT node and sub nodes of a controller on 
ConnectController call.
So we are not traversing the DT for runtime devices, rather we need to know if 
the device is runtime
or not when parsing the DT node.

Take the case of SPI NOR for example. When a SPI controller is connected, then 
instances for all the NOR flash devices
attached to controller are created. Only one of the NOR flash attached to SPI 
bus is going to be used for runtime service.

> 
> Note that *any* logic that operates on device trees that are provided by the 
> OS
> to the firmware will be rejected. It is the firmware's job to describe the 
> platform
> to the OS, not the other way around.

I have a question : if we want to use DT for DXE phase in firmware and to boot 
OS, should we use a common DT for firmware and OS or separate?

If DT HAS TO BE SAME, then how do we solve this conundrum ? 
https://elinux.org/Device_Tree_Linux#forward_and_backward_dts_compatibility
Current practice is that
•   new kernels work with old devicetrees
•   old kernels may or may not work with new devicetrees
This means that if a binding is modified in a non-compatible manner then the 
kernel implementation must still recognize the old binding until old 
devicetrees have been obsoleted and no longer exist.

i.e. a platform's DT is updated in linux 4.19 and I start to use it in UEFI. 
Then I try to boot linux 4.9 with it and it fails.

Regards,
Pankaj Bansal

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

[edk2] Uefi runtime property in device tree

[Pankaj Bansal]

Hello Ard et al.

I have a query regarding the device tree usage in UEFI.
In our UEFI implementation for NXP SOCs, we are using device tree to detect Non 
discoverable platform devices.
Based on the device detected in device tree, a device instance is created and 
the device's driver binds to that device's handle (a DXE driver or an UEFI 
driver).
if the device were to be used for runtime service, then we need to allocate the 
memory for that device instance from runtime pool and set its virtual address 
using EfiConvertPointer.
To facilitate this, I wish to add an optional property to the device node 
"uefi-runtime".
If this property is present in device tree the UEFI firmware will allocate the 
data from runtime pool for this device.
Also firmware will disable/delete the node in device tree before passing onto 
OS, so that OS doesn't use the device.

I wish to know your thoughts on this. If this doesn't seem the right way, I am 
happy to hear your suggestions.

Thanks & Regards,
Pankaj Bansal

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

[edk2] Uefi runtime property in device tree

[Pankaj Bansal]

Hello Ard et al.

I have a query regarding the device tree usage in UEFI.
In our UEFI implementation for NXP SOCs, we are using device tree to detect Non 
discoverable platform devices.
Based on the device detected in device tree, a device instance is created and 
the device's driver binds to that device's handle (a DXE driver or an UEFI 
driver).
if the device were to be used for runtime service, then we need to allocate the 
memory for that device instance from runtime pool and set its virtual address 
using EfiConvertPointer.
To facilitate this, I wish to add an optional property to the device node 
"uefi-runtime".
If this property is present in device tree the UEFI firmware will allocate the 
data from runtime pool for this device.
Also firmware will disable/delete the node in device tree before passing onto 
OS, so that OS doesn't use the device.

I wish to know your thoughts on this. If this doesn't seem the right way, I am 
happy to hear your suggestions.

Thanks & Regards,
Pankaj Bansal


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

Re: [edk2] PACKAGES_PATH in !include path in Dsc files

[Pankaj Bansal]

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Tuesday, October 16, 2018 10:58 AM
> To: Pankaj Bansal ; Ard Biesheuvel
> 
> Cc: Zhu, Yonghong ; Leif Lindholm
> ; Kinney, Michael D ;
> edk2-devel@lists.01.org; Udit Kumar ; Varun Sethi
> 
> Subject: RE: PACKAGES_PATH in !include path in Dsc files
> 
> Hi,
>   You can directly include it. BaseTools will search it from WORKSPACE and
> PACKAGES_PATH. So, you only need to set edk2-platforms directory into
> PACKAGES_PATH env.
> 
> !include Silicon/NXP/.dsc


Thanks You Liming Gao.
It worked for me.

> 
> Thanks
> Liming
> >-Original Message-
> >From: Pankaj Bansal [mailto:pankaj.ban...@nxp.com]
> >Sent: Tuesday, October 16, 2018 1:24 PM
> >To: Ard Biesheuvel 
> >Cc: Gao, Liming ; Zhu, Yonghong
> >; Leif Lindholm ;
> >Kinney, Michael D ;
> >edk2-devel@lists.01.org; Udit Kumar ; Varun Sethi
> >
> >Subject: RE: PACKAGES_PATH in !include path in Dsc files
> >
> >
> >
> >> -----Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: Tuesday, October 16, 2018 8:41 AM
> >> To: Pankaj Bansal 
> >> Cc: Gao, Liming ; Zhu, Yonghong
> >> ; Leif Lindholm ;
> >Michael
> >> D Kinney ; edk2-devel@lists.01.org; Udit
> >Kumar
> >> ; Varun Sethi 
> >> Subject: Re: PACKAGES_PATH in !include path in Dsc files
> >>
> >> On 16 October 2018 at 10:40, Pankaj Bansal 
> >wrote:
> >> > +edk2-platforms maintainers in To list
> >> >
> >> >
> >> >
> >> > Thank you Liming for replying.
> >> >
> >> >
> >> >
> >> > Our entire code is in edk2-platforms
> >> >
> >(https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >> > thub.com%2Ftianocore%2Fedk2-
> >>
> >platformsdata=02%7C01%7Cpankaj.bansal%40nxp.com%7C552da3f22b
> >5
> >>
> 84b7fac6008d63315ec8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> >>
> %7C636752566592695047sdata=JJWbAcZkj%2FtFaZC0bWONPb7ulCcj1
> >L2
> >> 4VKwCDGDx9OE%3Dreserved=0) which is denoted by
> >PACKAGES_PATH.
> >> >
> >> > The PACKAGES_PATH directory can be anywhere in WORKSPACE
> >depending on
> >> > the sync directory defined by user.
> >> >
> >> > i.e. it can be $(WORKSPACE)/edk2-platforms or $(WORKSPACE)/ >> > directory name that user can define during git sync>
> >> >
> >> > As our dsc files are relative to PACKAGES_PATH, I want to specify
> >> > their path in dsc file like this:
> >> >
> >> >
> >> >
> >> > !include $(PACKAGES_PATH)/Silicon/NXP/.dsc
> >> >
> >> >
> >> >
> >> > Using $(WORKSPACE), I cannot specify above path, as it can be at
> >> > place other than $(WORKSPACE)/edk2-platforms
> >> >
> >>
> >> But why do you need to !include things in the first place?
> >>
> >> Can you explain how you are trying to organize the files, and which
> >> file
> >includes
> >> which?
> >
> >I am trying to keep Silicon (SOC) specific dsc file in
> >Silicon/NXP/ >Name>/
> >This silicon can be used in multiple Boards (Platforms).
> >All these Platforms are present in Platform/NXP/ fd/fv
> >binaries would be created for each platform.
> >The chassis dsc file has description of components/PCDs that are
> >specific to chassis to which the silicon belongs. It would be same for
> >all silicons that belong to same chassis.
> >The Silicon dsc file has description of components/PCDs that are
> >specific to silicon and would be same for all platforms that use this
> >silicon. It would include chassis dsc file The Platform dsc file would
> >include the silicon dsc file.
> >
> >___
> >| Platform (in Platform/NXP)|
> >|_  |
> >|   | Silicon (in Silicon/NXP/) | |
> >|   |   ___| |
> >|   |  | Chassis  (in Silicon/NXP) |   ||
> >|   |  |__|   | |
> >|   || |
> >|_|
> >
> >Regards,
> >Pankaj Bansal
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] PACKAGES_PATH in !include path in Dsc files

[Pankaj Bansal]

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, October 16, 2018 8:41 AM
> To: Pankaj Bansal 
> Cc: Gao, Liming ; Zhu, Yonghong
> ; Leif Lindholm ; Michael
> D Kinney ; edk2-devel@lists.01.org; Udit Kumar
> ; Varun Sethi 
> Subject: Re: PACKAGES_PATH in !include path in Dsc files
> 
> On 16 October 2018 at 10:40, Pankaj Bansal  wrote:
> > +edk2-platforms maintainers in To list
> >
> >
> >
> > Thank you Liming for replying.
> >
> >
> >
> > Our entire code is in edk2-platforms
> > (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > thub.com%2Ftianocore%2Fedk2-
> platformsdata=02%7C01%7Cpankaj.bansal%40nxp.com%7C552da3f22b5
> 84b7fac6008d63315ec8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636752566592695047sdata=JJWbAcZkj%2FtFaZC0bWONPb7ulCcj1L2
> 4VKwCDGDx9OE%3Dreserved=0) which is denoted by PACKAGES_PATH.
> >
> > The PACKAGES_PATH directory can be anywhere in WORKSPACE depending on
> > the sync directory defined by user.
> >
> > i.e. it can be $(WORKSPACE)/edk2-platforms or $(WORKSPACE)/ > directory name that user can define during git sync>
> >
> > As our dsc files are relative to PACKAGES_PATH, I want to specify
> > their path in dsc file like this:
> >
> >
> >
> > !include $(PACKAGES_PATH)/Silicon/NXP/.dsc
> >
> >
> >
> > Using $(WORKSPACE), I cannot specify above path, as it can be at place
> > other than $(WORKSPACE)/edk2-platforms
> >
> 
> But why do you need to !include things in the first place?
> 
> Can you explain how you are trying to organize the files, and which file 
> includes
> which?

I am trying to keep Silicon (SOC) specific dsc file in Silicon/NXP//
This silicon can be used in multiple Boards (Platforms).
All these Platforms are present in Platform/NXP/
fd/fv binaries would be created for each platform.
The chassis dsc file has description of components/PCDs that are specific to 
chassis to which the silicon belongs. It would be same for all silicons that 
belong to same chassis.
The Silicon dsc file has description of components/PCDs that are specific to 
silicon and would be same for all platforms that use this silicon. It would 
include chassis dsc file
The Platform dsc file would include the silicon dsc file.

___
| Platform (in Platform/NXP)|
|_  |
|   | Silicon (in Silicon/NXP/) | |
|   |   ___| |
|   |  | Chassis  (in Silicon/NXP) |   ||
|   |  |__|   | |
|   || |
|_|

Regards,
Pankaj Bansal
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] PACKAGES_PATH in !include path in Dsc files

[Pankaj Bansal]

+edk2-platforms maintainers in To list

Thank you Liming for replying.

Our entire code is in edk2-platforms 
(https://github.com/tianocore/edk2-platforms) which is denoted by PACKAGES_PATH.
The PACKAGES_PATH directory can be anywhere in WORKSPACE depending on the sync 
directory defined by user.
i.e. it can be $(WORKSPACE)/edk2-platforms or $(WORKSPACE)/
As our dsc files are relative to PACKAGES_PATH, I want to specify their path in 
dsc file like this:

!include $(PACKAGES_PATH)/Silicon/NXP/.dsc

Using $(WORKSPACE), I cannot specify above path, as it can be at place other 
than $(WORKSPACE)/edk2-platforms

Regards,
Pankaj Bansal

From: Gao, Liming [mailto:liming@intel.com]
Sent: Tuesday, October 16, 2018 7:06 AM
To: Pankaj Bansal ; Zhu, Yonghong 

Cc: edk2-devel@lists.01.org; Udit Kumar ; Varun Sethi 

Subject: RE: PACKAGES_PATH in !include path in Dsc files

What's your usage model in DSC?

BaseTools will try to replace $(WORKSPACE) with WORKSPACE and PACKAGES_PATH, 
and find the first existing file. So, if you want to refer to one file in 
PACKAGES_PATH directory, you can also use $(WORKSPACE) macro to refer to it.

Thanks
Liming
From: Pankaj Bansal [mailto:pankaj.ban...@nxp.com]
Sent: Saturday, October 13, 2018 5:01 PM
To: Zhu, Yonghong mailto:yonghong@intel.com>>; Gao, 
Liming mailto:liming@intel.com>>
Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Udit Kumar 
mailto:udit.ku...@nxp.com>>; Varun Sethi 
mailto:v.se...@nxp.com>>
Subject: PACKAGES_PATH in !include path in Dsc files

Hello All,

I am trying to add this functionality that we can specify PACKAGES_PATH in 
!include path in Dsc files just like we can specify WORKSPACE.
I did below changes for it:

--- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
+++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
@@ -1530,6 +1530,7 @@ class DscParser(MetaFileParser):
 # Allow using system environment variables  in path after !include
 #
 __IncludeMacros['WORKSPACE'] = 
GlobalData.gGlobalDefines['WORKSPACE']
+__IncludeMacros['PACKAGES_PATH'] = 
GlobalData.gGlobalDefines['PACKAGES_PATH']^M
 if "ECP_SOURCE" in GlobalData.gGlobalDefines:
 __IncludeMacros['ECP_SOURCE'] = 
GlobalData.gGlobalDefines['ECP_SOURCE']
 #
diff --git a/BaseTools/Source/Python/build/build.py 
b/BaseTools/Source/Python/build/build.py
index d74082fc26..61dce3a856 100644
--- a/BaseTools/Source/Python/build/build.py
+++ b/BaseTools/Source/Python/build/build.py
@@ -197,6 +197,7 @@ def CheckEnvVariable():
 GlobalData.gEcpSource = EcpSourceDir

 GlobalData.gGlobalDefines["WORKSPACE"]  = WorkspaceDir
+GlobalData.gGlobalDefines["PACKAGES_PATH"]  = PackagesPath^M
 GlobalData.gGlobalDefines["EFI_SOURCE"] = EfiSourceDir
 GlobalData.gGlobalDefines["EDK_SOURCE"] = EdkSourceDir
 GlobalData.gGlobalDefines["ECP_SOURCE"] = EcpSourceDir

With these changes the compilation starts OK, but I get this error later on:

GenFds.py...
: error C0DE: Tools code failure
Please send email to 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> for help, attaching 
following call stack trace!

Traceback (most recent call last):
  File "/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/GenFds/GenFds.py", 
line 246, in main
TargetArchList = 
set(BuildWorkSpace.BuildObject[GenFdsGlobalVariable.ActivePlatform, TAB_COMMON, 
Options.BuildTarget, Options.ToolChain].SupArchList) & set(ArchList)
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py",
 line 132, in __getitem__
Toolchain
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/DscBuildData.py",
 line 221, in __init__
self._HandleOverridePath()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/DscBuildData.py",
 line 282, in _HandleOverridePath
RecordList = self._RawData[MODEL_META_DATA_COMPONENT, self._Arch]
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 257, in __getitem__
self._PostProcess()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 1358, in _PostProcess
Processer[self._ItemType]()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 1533, in __ProcessDirective
__IncludeMacros['PACKAGES_PATH'] = 
GlobalData.gGlobalDefines['PACKAGES_PATH']
KeyError: 'PACKAGES_PATH'



build.py...
: error 7000: Failed to execute command
GenFds -f 
/home/nxa34148/Desktop/uefi/edk2-platforms/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
 --conf=/home/nxa34148/Desktop/uefi/Conf -o 
/home/nxa34148/Desktop/uefi/Build/LS1043aRdbPkg/RELEASE_GCC49 -t GCC49 -b 
RELEASE -p 
/home/nxa34148/Desktop/uefi/edk2

[edk2] PACKAGES_PATH in !include path in Dsc files

[Pankaj Bansal]

Hello All,

I am trying to add this functionality that we can specify PACKAGES_PATH in 
!include path in Dsc files just like we can specify WORKSPACE.
I did below changes for it:

--- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
+++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
@@ -1530,6 +1530,7 @@ class DscParser(MetaFileParser):
 # Allow using system environment variables  in path after !include
 #
 __IncludeMacros['WORKSPACE'] = 
GlobalData.gGlobalDefines['WORKSPACE']
+__IncludeMacros['PACKAGES_PATH'] = 
GlobalData.gGlobalDefines['PACKAGES_PATH']^M
 if "ECP_SOURCE" in GlobalData.gGlobalDefines:
 __IncludeMacros['ECP_SOURCE'] = 
GlobalData.gGlobalDefines['ECP_SOURCE']
 #
diff --git a/BaseTools/Source/Python/build/build.py 
b/BaseTools/Source/Python/build/build.py
index d74082fc26..61dce3a856 100644
--- a/BaseTools/Source/Python/build/build.py
+++ b/BaseTools/Source/Python/build/build.py
@@ -197,6 +197,7 @@ def CheckEnvVariable():
 GlobalData.gEcpSource = EcpSourceDir

 GlobalData.gGlobalDefines["WORKSPACE"]  = WorkspaceDir
+GlobalData.gGlobalDefines["PACKAGES_PATH"]  = PackagesPath^M
 GlobalData.gGlobalDefines["EFI_SOURCE"] = EfiSourceDir
 GlobalData.gGlobalDefines["EDK_SOURCE"] = EdkSourceDir
 GlobalData.gGlobalDefines["ECP_SOURCE"] = EcpSourceDir

With these changes the compilation starts OK, but I get this error later on:

GenFds.py...
: error C0DE: Tools code failure
Please send email to edk2-devel@lists.01.org for help, attaching 
following call stack trace!

Traceback (most recent call last):
  File "/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/GenFds/GenFds.py", 
line 246, in main
TargetArchList = 
set(BuildWorkSpace.BuildObject[GenFdsGlobalVariable.ActivePlatform, TAB_COMMON, 
Options.BuildTarget, Options.ToolChain].SupArchList) & set(ArchList)
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py",
 line 132, in __getitem__
Toolchain
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/DscBuildData.py",
 line 221, in __init__
self._HandleOverridePath()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/DscBuildData.py",
 line 282, in _HandleOverridePath
RecordList = self._RawData[MODEL_META_DATA_COMPONENT, self._Arch]
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 257, in __getitem__
self._PostProcess()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 1358, in _PostProcess
Processer[self._ItemType]()
  File 
"/home/nxa34148/Desktop/uefi/BaseTools/Source/Python/Workspace/MetaFileParser.py",
 line 1533, in __ProcessDirective
__IncludeMacros['PACKAGES_PATH'] = 
GlobalData.gGlobalDefines['PACKAGES_PATH']
KeyError: 'PACKAGES_PATH'



build.py...
: error 7000: Failed to execute command
GenFds -f 
/home/nxa34148/Desktop/uefi/edk2-platforms/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.fdf
 --conf=/home/nxa34148/Desktop/uefi/Conf -o 
/home/nxa34148/Desktop/uefi/Build/LS1043aRdbPkg/RELEASE_GCC49 -t GCC49 -b 
RELEASE -p 
/home/nxa34148/Desktop/uefi/edk2-platforms/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
 -a AARCH64 -D "EFI_SOURCE=/home/nxa34148/Desktop/uefi/EdkCompatibilityPkg" -D 
"EDK_SOURCE=/home/nxa34148/Desktop/uefi/EdkCompatibilityPkg" -D 
"TOOL_CHAIN_TAG=GCC49" -D "TOOLCHAIN=GCC49" -D "FAMILY=GCC" -D 
"PACKAGES_PATH=/home/nxa34148/Desktop/uefi/edk2-platforms" -D 
"EDK_TOOLS_PATH=/home/nxa34148/Desktop/uefi/BaseTools" -D 
"WORKSPACE=/home/nxa34148/Desktop/uefi" -D "ARCH=AARCH64" -D 
"ECP_SOURCE=/home/nxa34148/Desktop/uefi/EdkCompatibilityPkg" -D 
"TARGET=RELEASE" [/home/nxa34148/Desktop/uefi]

- Failed -

I am not able to understand the cause of this error.

Can you please help?

Regards,
Pankaj Bansal
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] Add Platform Include path in modules

[Pankaj Bansal]

Hi All,

Will It violate the UEFI Platform Initialization Distribution Packaging 
Specification if we want to use Computed Includes ?
https://gcc.gnu.org/onlinedocs/gcc-3.0.2/cpp_2.html#SEC10

Thanks & Regards,
Pankaj Bansal

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Tuesday, February 27, 2018 4:52 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Laszlo Ersek <ler...@redhat.com>; edk2-
> de...@lists.01.org
> Subject: RE: [edk2] [RFC] Add Platform Include path in modules
> 
> Hi,
>   For the first one, the same PCD can be configured to the different value for
> the different SkuId.
> 
> Here is wiki on structure pcd enable step and examples.
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Flgao4%2Fedk2%2Fwiki%2FStructurePcd-Enable-
> Steps=02%7C01%7Cpankaj.bansal%40nxp.com%7C613339ac821e4bcc9
> fd508d57dd44781%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 36553273075487313=DNiyfbhwN%2BKdFkltL%2FQQTzWFc8hhnNsXbC
> 4KjIHUy%2Bs%3D=0
> 
> TestPkg in
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Flgao4%2Fedk2%2Ftree%2FUDK2018=02%7C01%7Cpankaj.b
> ansal%40nxp.com%7C613339ac821e4bcc9fd508d57dd44781%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636553273075487313=8aBhP
> vNDkt%2FPb2kmAEXz0gERW9MFGD3kt8YcvjGi7jc%3D=0 has the
> sample case with structure pcd.
> 
> Thanks
> Liming
> >-Original Message-
> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >Pankaj Bansal
> >Sent: Tuesday, February 27, 2018 3:40 PM
> >To: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek
> ><ler...@redhat.com>; edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming@intel.com>
> >Subject: Re: [edk2] [RFC] Add Platform Include path in modules
> >
> >Hi Laszlo/Michael,
> >
> >Thanks for your feedback on this proposal.
> >I looked at the structured PCDs and UEFI Platform Initialization
> >Distribution Packaging Specification.
> >Here is my take on these.
> >
> >1. structured PCDs are good if we want to declare single complex structure.
> >But consider a case where I want to keep device information in structure.
> >(e.g. hardware settings, limitations etc)
> >And we may want to tweak this information based on platform
> >revision being used.
> >And different platforms can have different number of such devices.
> >In this case, when we want to add a new platform, we might need to
> >introduce new PCDs in .dec files, which will not be needed for others.
> >I don't know, will this even increase the PCD database size for
> >existing platforms or not ?
> >
> >2. To mitigate the "hidden" dependency of a module on platform, we can
> >explicitly declare this dependency in module inf file.
> > I am thinking something like gEfiCallerIdGuid, i.e. module can
> >declare that that platform building(using) the module, supply this
> information.
> >
> >3. Using Libraries and Protocols can also solve such use cases. I just
> >felt that it's less cumbersome to use include files, and it also avoids code
> replication.
> >Anyway, this is just my suggestion to have such mechanism in edk2
> >build process. I am more than happy to stick to platform libraries.
> >
> >Thanks & Regards,
> >Pankaj Bansal
> >
> >> -Original Message-
> >> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> >> Sent: Monday, February 26, 2018 10:56 PM
> >> To: Laszlo Ersek <ler...@redhat.com>; Pankaj Bansal
> >> <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org; Kinney, Michael D
> >> <michael.d.kin...@intel.com>
> >> Cc: Gao, Liming <liming@intel.com>
> >> Subject: RE: [edk2] [RFC] Add Platform Include path in modules
> >>
> >> Hi Pankaj,
> >>
> >> I agree with Laszlo that you should evaluate use of PCDs.  There are
> >> a few methods for a driver to use platform specific values/behavior.
> These are:
> >>
> >> * PCDs
> >> * Library class/Library Instance
> >> * Protocol/PPI
> >>
> >> One issue with the proposal is that it adds a hidden dependency to
> modules.
> >> An EDK II INF file describes the external interfaces of a module
> >> along with produces/consumes usage.  This information is aligned with
> >> the XML
> >schema
> >> that is documented in the UEFI Platform Initialization Distribution
> &

Re: [edk2] [RFC] Add Platform Include path in modules

[Pankaj Bansal]

Hi Laszlo/Michael,

Thanks for your feedback on this proposal.
I looked at the structured PCDs and UEFI Platform Initialization Distribution 
Packaging Specification.
Here is my take on these.

1. structured PCDs are good if we want to declare single complex structure.
But consider a case where I want to keep device information in structure. 
(e.g. hardware settings, limitations etc)
And we may want to tweak this information based on platform revision being 
used.
And different platforms can have different number of such devices.
In this case, when we want to add a new platform, we might need to 
introduce new PCDs in .dec files, which will not be needed for others.
I don't know, will this even increase the PCD database size for existing 
platforms or not ?

2. To mitigate the "hidden" dependency of a module on platform, we can 
explicitly declare this dependency in module inf file.
 I am thinking something like gEfiCallerIdGuid, i.e. module can declare 
that that platform building(using) the module, supply this information.

3. Using Libraries and Protocols can also solve such use cases. I just felt 
that it's less cumbersome to use include files, and it also avoids code 
replication.
Anyway, this is just my suggestion to have such mechanism in edk2 build 
process. I am more than happy to stick to platform libraries.

Thanks & Regards,
Pankaj Bansal

> -Original Message-
> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> Sent: Monday, February 26, 2018 10:56 PM
> To: Laszlo Ersek <ler...@redhat.com>; Pankaj Bansal
> <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: Gao, Liming <liming@intel.com>
> Subject: RE: [edk2] [RFC] Add Platform Include path in modules
> 
> Hi Pankaj,
> 
> I agree with Laszlo that you should evaluate use of PCDs.  There are a few
> methods for a driver to use platform specific values/behavior.  These are:
> 
> * PCDs
> * Library class/Library Instance
> * Protocol/PPI
> 
> One issue with the proposal is that it adds a hidden dependency to modules.
> An EDK II INF file describes the external interfaces of a module along with
> produces/consumes usage.  This information is aligned with the XML schema
> that is documented in the UEFI Platform Initialization Distribution Packaging
> Specification.
> 
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fuefi.
> org%2Fspecifications=02%7C01%7Cpankaj.bansal%40nxp.com%7C96c4
> 2dd7271d449d56e908d57d3df2d4%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636552627376357192=sS7KT3haANF5TLHSeRGjIQHnLQ
> BtnTDLIZWntUhsk78%3D=0
> 
> If two modules have the same GUID/Version, then the external interfaces to
> those two modules are expected to be identical.
> With your proposal, two modules built for 2 different platforms would have
> the same GUID/Version but would not have the same external interfaces
> because a hidden dependency on a platform package was added.
> 
> If a module really needs to use content from a platform package, then a new
> copy of the module should be created with a new GUID/Version and the
> platform package added to the [Packages] section.  The other option is to use
> one of the supported interfaces (PCDs, Lib, Protocol, PPI).
> 
> Please let us know if any of these exiting methods do not work for your use
> case.
> 
> Thanks,
> 
> Mike
> 
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Monday, February 26, 2018 7:55 AM
> > To: Pankaj Bansal <pankaj.ban...@nxp.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; edk2- de...@lists.01.org
> > Cc: Gao, Liming <liming@intel.com>
> > Subject: Re: [edk2] [RFC] Add Platform Include path in modules
> >
> > On 02/26/18 11:55, Pankaj Bansal wrote:
> > > Hi,
> > >
> > > Consider a simple driver which needs that some data
> > structures be
> > > filled by the Platform, which is using the driver.
> > >
> > > Driver.c #include 
> > >
> > > Struct a = platformVal;
> > >
> > > We can define platformVal in Platform.h, which would
> > be unique to the
> > > platform being built. This Platform.h can be placed in
> > include
> > > directories, whose path would be defined in
> > Platform.dec file.
> > >
> > > Now, whenever we build driver for each unique
> > platform, we need not
> > > to mention Platform.dec file in driver.inf [packages]
> > section. We can
> > > append Platform.dec include paths to each driver. i.e.
> > look for the
> > > include files in [packages] sect

[edk2] [RFC v2] Add Platform Include path in modules

[Pankaj Bansal]

When we are writing the drivers for IP modules, then sometimes we want
that Platform specific customizations or platform dependent values be
supplied to IP module driver. normally we achieve this using Pcd values.

But sometimes we want to use header files for such data.e.g. if the
values are complex structures.

we need a mechanism that platform be able to supply these header files
to a module, without changing module code.

This patch is an attempt to achieve this functionality.

Cc: Yonghong Zhu <yonghong@intel.com>
Cc: Liming Gao <liming@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---

Notes:
V2:
 -check if .dec file exist before appending include paths to include path 
list

 BaseTools/Source/Python/AutoGen/AutoGen.py | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 405bfa1..4b281d8 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -3783,6 +3783,20 @@ class ModuleAutoGen(AutoGen):
 for Inc in IncludesList:
 if Inc not in self._IncludePathList:
 self._IncludePathList.append(str(Inc))
+PackageFilePath = os.path.join(self.PlatformInfo.MetaFile.SubDir, 
self.PlatformInfo.MetaFile.BaseName + '.dec')
+if 
os.path.isfile(os.path.normpath(mws.join(self.PlatformInfo.MetaFile.Root, 
PackageFilePath))):
+PackageFile = PathClass(PackageFilePath, 
self.PlatformInfo.MetaFile.Root)
+Package = self.BuildDatabase[PackageFile, self.Arch, 
self.BuildTarget, self.ToolChain]
+PackageDir = mws.join(self.WorkspaceDir, Package.MetaFile.Dir)
+if PackageDir not in self._IncludePathList:
+self._IncludePathList.append(PackageDir)
+IncludesList = Package.Includes
+if Package._PrivateIncludes:
+if not self.MetaFile.Path.startswith(PackageDir):
+IncludesList = 
list(set(Package.Includes).difference(set(Package._PrivateIncludes)))
+for Inc in IncludesList:
+if Inc not in self._IncludePathList:
+self._IncludePathList.append(str(Inc))
 return self._IncludePathList
 
 def _GetIncludePathLength(self):
-- 
2.7.4

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

Re: [edk2] [RFC] Add Platform Include path in modules

[Pankaj Bansal]

Hi,

Consider a simple driver which needs that some data structures be filled by the
Platform, which is using the driver.

Driver.c
#include 

Struct a = platformVal;

We can define platformVal in Platform.h, which would be unique to the platform 
being built.
This Platform.h can be placed in include directories, whose path would be 
defined in Platform.dec file.

Now, whenever we build driver for each unique platform, we need not to mention 
Platform.dec file in driver.inf [packages] section.
We can append Platform.dec include paths to each driver. i.e. look for the 
include files in [packages] section as well as in Platform include directories.

For this, I am looking for Platform.dec file in same directory as Platform.dsc 
and using same name as Platform.dsc

We can refine this change further. i.e. add Platform include directories to 
driver's include paths based on some condition in driver.inf file.

Thanks & Regards,
Pankaj Bansal

> -Original Message-
> From: Kinney, Michael D [mailto:michael.d.kin...@intel.com]
> Sent: Monday, February 26, 2018 1:13 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Gao, Liming <liming@intel.com>
> Subject: RE: [edk2] [RFC] Add Platform Include path in modules
> 
> Hi,
> 
> Can you provide a simple example that shows how this feature is used and
> how it works?
> 
> Thanks,
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Pankaj Bansal
> > Sent: Sunday, February 25, 2018 10:29 PM
> > To: edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming@intel.com>
> > Subject: [edk2] [RFC] Add Platform Include path in modules
> >
> > When we are writing the drivers for IP modules, then sometimes we want
> > that Platform specific customizations or platform dependent values be
> > supplied to IP module driver. normally we achieve this using Pcd
> > values.
> >
> > But sometimes we want to use header files for such data.e.g. if the
> > values are complex structures.
> >
> > we need a mechanism that platform be able to supply these header files
> > to a module, without changing module code.
> >
> > This patch is an attempt to achieve this functionality.
> >
> > Cc: Yonghong Zhu <yonghong@intel.com>
> > Cc: Liming Gao <liming@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> > ---
> >  BaseTools/Source/Python/AutoGen/AutoGen.py | 12
> > 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
> > b/BaseTools/Source/Python/AutoGen/AutoGen.py
> > index 405bfa1..de4a17c 100644
> > --- a/BaseTools/Source/Python/AutoGen/AutoGen.py
> > +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
> > @@ -3783,6 +3783,18 @@ class ModuleAutoGen(AutoGen):
> >  for Inc in IncludesList:
> >  if Inc not in
> > self._IncludePathList:
> >
> > self._IncludePathList.append(str(Inc))
> > +PackageFile =
> > PathClass(os.path.join(self.PlatformInfo.MetaFile.SubDir
> > , self.PlatformInfo.MetaFile.BaseName + '.dec'),
> > self.PlatformInfo.MetaFile.Root)
> > +Package = self.BuildDatabase[PackageFile,
> > self.Arch, self.BuildTarget, self.ToolChain]
> > +PackageDir = mws.join(self.WorkspaceDir,
> > Package.MetaFile.Dir)
> > +if PackageDir not in self._IncludePathList:
> > +
> > self._IncludePathList.append(PackageDir)
> > +IncludesList = Package.Includes
> > +if Package._PrivateIncludes:
> > +if not
> > self.MetaFile.Path.startswith(PackageDir):
> > +IncludesList =
> > list(set(Package.Includes).difference(set(Package._Priva
> > teIncludes)))
> > +for Inc in IncludesList:
> > +if Inc not in self._IncludePathList:
> > +
> > self._IncludePathList.append(str(Inc))
> >  return self._IncludePathList
> >
> >  def _GetIncludePathLength(self):
> > --
> > 2.7.4
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel=02%7C01%7Cpankaj.bans
> >
> al%40nxp.com%7Ccaec1b6b479b4111c26308d57cec86ac%7C686ea1d3bc2b4
> c6fa92c
> >
> d99c5c301635%7C0%7C0%7C636552277685045614=G3VP7FkkVcBKN4
> bkA5RRdJ
> > FzfpdIBhuxmRmBMcFvMY8%3D=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] MdePkg/BaseLib: Change BitField functions.

[Pankaj Bansal]

Hello Laszlo,

Thanks for your explanation.
Now I understand the usage of bit field functions.

Regards,
Pankaj Bansal

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 5:43 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Gao, Liming
> <liming@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> Subject: Re: [edk2] [RFC] MdePkg/BaseLib: Change BitField functions.
> 
> On 02/23/18 12:16, Pankaj Bansal wrote:
> > Hi Liming Gao,
> >
> > Thanks for your comments.
> > I am not able to understand the full purpose of the BitField functions.
> > Can you please help me to understand.
> >
> > As per my understanding :
> > BitFieldAnd32 (Operand, StartBit, EndBit, AndData) :
> >
> > We want to calculate the bitwise and (&) between Operand's bits (StartBit
> to EndBit, both inclusive) and AndData's bits (Startbit to Endbit, both
> inclusive).
> > If this is the case, then why is there a restriction on AndData ? (If
> > AndData is larger than the bitmask value range specified by StartBit
> > and EndBit, then ASSERT().)
> >
> > How these functions are intended to be used ?
> 
> StartBit and EndBit designate a bit-field within Operand. Operand is the
> "container" of the bit-field.
> 
> "AndData" is meant for the bit-field itself, not the container. AndData is
> applied relative to StartBit, and (relative to StartBit) it must not exceed
> EndBit.
> 
> So BitFieldAnd32() applies the AndData mask to the bit-field within the 32-bit
> container word. Bits outside of the bit-field are unchanged.
> 
> Laszlo
> 
> 
> >> -Original Message-
> >> From: Gao, Liming [mailto:liming@intel.com]
> >> Sent: Friday, February 23, 2018 4:33 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> >> Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions.
> >>
> >> Pankaj:
> >>   OrData, AndData and Value are the bit field value specified by
> >> StartBit and EndBit. They are not the full value. They must be
> >> adjusted to the full value, then calculated with Operand.
> >>
> >>   And, we use LShit() or RShit() function for 64bit operand, because
> >> we find VS compiler may generate the intrinsic function when >> or <<
> >> is used 64bit operand on 32bit arch.
> >>
> >> Thanks
> >> Liming
> >>> -Original Message-
> >>> From: Pankaj Bansal [mailto:pankaj.ban...@nxp.com]
> >>> Sent: Tuesday, February 20, 2018 12:50 PM
> >>> To: edk2-devel@lists.01.org
> >>> Cc: Gao, Liming <liming@intel.com>; Kinney, Michael D
> >>> <michael.d.kin...@intel.com>
> >>> Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions.
> >>>
> >>> Hi everybody,
> >>>
> >>> Any comments on this change ?
> >>>
> >>>> -Original Message-
> >>>> From: Pankaj Bansal
> >>>> Sent: Friday, February 09, 2018 4:30 PM
> >>>> To: edk2-devel@lists.01.org
> >>>> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>; Michael D Kinney
> >>>> <michael.d.kin...@intel.com>; Liming Gao <liming@intel.com>
> >>>> Subject: [RFC] MdePkg/BaseLib: Change BitField functions.
> >>>>
> >>>> The bit field functions in MdePkg are not working as expected.
> >>>> The restrictions on Value parameter such that Value should not be
> >>>> greater than the bitmask value range specified by StartBit and
> >>>> EndBit doesn't seem
> >>> to
> >>>> make sense.
> >>>>
> >>>> Also the restriction on End bit to not be equal to start bit
> >>>> prohibits single bit change.
> >>>>
> >>>> This is an attempt to correct these limitations.
> >>>>
> >>>> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> >>>> Cc: Liming Gao <liming@intel.com>
> >>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> >>>> ---
> >>>>
> >>>> Notes:
> >>>> This is a RFC patch.
> >>>>
> >>>>  MdePkg/Library/BaseLib/BitField.c | 179
> >&

[edk2] [RFC] Add Platform Include path in modules

[Pankaj Bansal]

When we are writing the drivers for IP modules, then sometimes we want
that Platform specific customizations or platform dependent values be
supplied to IP module driver. normally we achieve this using Pcd values.

But sometimes we want to use header files for such data.e.g. if the
values are complex structures.

we need a mechanism that platform be able to supply these header files
to a module, without changing module code.

This patch is an attempt to achieve this functionality.

Cc: Yonghong Zhu <yonghong@intel.com>
Cc: Liming Gao <liming@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
 BaseTools/Source/Python/AutoGen/AutoGen.py | 12 
 1 file changed, 12 insertions(+)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 405bfa1..de4a17c 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -3783,6 +3783,18 @@ class ModuleAutoGen(AutoGen):
 for Inc in IncludesList:
 if Inc not in self._IncludePathList:
 self._IncludePathList.append(str(Inc))
+PackageFile = 
PathClass(os.path.join(self.PlatformInfo.MetaFile.SubDir, 
self.PlatformInfo.MetaFile.BaseName + '.dec'), self.PlatformInfo.MetaFile.Root)
+Package = self.BuildDatabase[PackageFile, self.Arch, 
self.BuildTarget, self.ToolChain]
+PackageDir = mws.join(self.WorkspaceDir, Package.MetaFile.Dir)
+if PackageDir not in self._IncludePathList:
+self._IncludePathList.append(PackageDir)
+IncludesList = Package.Includes
+if Package._PrivateIncludes:
+if not self.MetaFile.Path.startswith(PackageDir):
+IncludesList = 
list(set(Package.Includes).difference(set(Package._PrivateIncludes)))
+for Inc in IncludesList:
+if Inc not in self._IncludePathList:
+self._IncludePathList.append(str(Inc))
 return self._IncludePathList
 
 def _GetIncludePathLength(self):
-- 
2.7.4

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

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:52 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Udit Kumar
> <udit.ku...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 12:04, Pankaj Bansal wrote:
> 
> > However Laszlo, with the method you suggest (using STATIC CONST
> > UINT16 mOne), would it not add delay in each Mmio Operation ?
> >
> > I am concerned about boot delay using this approach.
> The condition can be evaluated at compile time, so I expect optimizing
> compilers to eliminate the dead branch.
> 
> Assuming the condition cannot be eliminated at build time, what is your
> concern: the single byte access, or the branch instruction?
> 
> I don't think the single byte access matters. (If you tried to replace that 
> with a
> HOB or PCD lookup, it could only be worse.)
> 
> I also doubt the branch should be a concern. You could replace the "if"
> (or the ternary operator "?:") with function pointers that you set e.g.
> in a constructor function. But I think an "if" with an invariable
> (constant) controlling expression is at least as friendly towards branch
> predictors as a function pointer variable (through which you might be
> *forced* to make a real function call).
> 
> Personally I wouldn't worry.
> 

I think you are right about smart compiler eliminating the branches at build 
time.
I just pointed this out because we call Mmio/BeMmio APIs when accessing Nor 
flash for variable read/write.
As these are called so many time during boot, I did not want any delay to be 
added to these APIs than necessary.
Now that you have pointed it out, I don't think any significant delay will be 
added to these APIs.

> 
> Anyway: please do not mistake my willingness / preference to go into such
> detail for having high stakes at this. If you go an entirely different route, 
> I'm
> OK with that too. I felt I was asked for my opinion and I tried to express it 
> in
> detail, that's all.

Any comments/suggestions/opinions are always appreciated from you or from any 
edk2 mailing list member.

> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC] MdePkg/BaseLib: Change BitField functions.

[Pankaj Bansal]

Hi Liming Gao,

Thanks for your comments.
I am not able to understand the full purpose of the BitField functions.
Can you please help me to understand.

As per my understanding :
BitFieldAnd32 (Operand, StartBit, EndBit, AndData) :

We want to calculate the bitwise and (&) between Operand's bits (StartBit to 
EndBit, both inclusive) and AndData's bits (Startbit to Endbit, both inclusive).
If this is the case, then why is there a restriction on AndData ? (If AndData 
is larger than the bitmask value range specified by StartBit and EndBit, then 
ASSERT().)

How these functions are intended to be used ?

Thanks & Regards,
Pankaj Bansal

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Friday, February 23, 2018 4:33 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions.
> 
> Pankaj:
>   OrData, AndData and Value are the bit field value specified by StartBit and
> EndBit. They are not the full value. They must be adjusted to the full value,
> then calculated with Operand.
> 
>   And, we use LShit() or RShit() function for 64bit operand, because we find
> VS compiler may generate the intrinsic function when >> or << is used 64bit
> operand on 32bit arch.
> 
> Thanks
> Liming
> >-Original Message-
> >From: Pankaj Bansal [mailto:pankaj.ban...@nxp.com]
> >Sent: Tuesday, February 20, 2018 12:50 PM
> >To: edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming@intel.com>; Kinney, Michael D
> ><michael.d.kin...@intel.com>
> >Subject: RE: [RFC] MdePkg/BaseLib: Change BitField functions.
> >
> >Hi everybody,
> >
> >Any comments on this change ?
> >
> >> -Original Message-
> >> From: Pankaj Bansal
> >> Sent: Friday, February 09, 2018 4:30 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>; Michael D Kinney
> >> <michael.d.kin...@intel.com>; Liming Gao <liming@intel.com>
> >> Subject: [RFC] MdePkg/BaseLib: Change BitField functions.
> >>
> >> The bit field functions in MdePkg are not working as expected.
> >> The restrictions on Value parameter such that Value should not be
> >> greater than the bitmask value range specified by StartBit and EndBit
> >> doesn't seem
> >to
> >> make sense.
> >>
> >> Also the restriction on End bit to not be equal to start bit
> >> prohibits single bit change.
> >>
> >> This is an attempt to correct these limitations.
> >>
> >> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> >> Cc: Liming Gao <liming@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> >> ---
> >>
> >> Notes:
> >> This is a RFC patch.
> >>
> >>  MdePkg/Library/BaseLib/BitField.c | 179 ++--
> >>  1 file changed, 89 insertions(+), 90 deletions(-)
> >>
> >> diff --git a/MdePkg/Library/BaseLib/BitField.c
> >> b/MdePkg/Library/BaseLib/BitField.c
> >> index eb9e276..2b5be52 100644
> >> --- a/MdePkg/Library/BaseLib/BitField.c
> >> +++ b/MdePkg/Library/BaseLib/BitField.c
> >> @@ -2,6 +2,8 @@
> >>Bit field functions of BaseLib.
> >>
> >>Copyright (c) 2006 - 2013, Intel Corporation. All rights
> >> reserved.
> >> +  Copyright 2018 NXP
> >> +
> >>This program and the accompanying materials
> >>are licensed and made available under the terms and conditions of
> >> the BSD License
> >>which accompanies this distribution.  The full text of the license
> >> may be found at @@ -14,6 +16,12 @@
> >>
> >>  #include "BaseLibInternals.h"
> >>
> >> +#define BITMASK_UNITN(StartBit, EndBit) \
> >> +(((MAX_UINTN) << (StartBit)) & (MAX_UINTN >> ((sizeof (UINTN) *
> >> +8)
> >> +- 1 - (EndBit
> >> +
> >> +#define BITMASK_UNIT64(StartBit, EndBit) \
> >> +(((MAX_UINT64) << (StartBit)) & (MAX_UINT64 >> ((sizeof (UINT64)
> >> +*
> >> +8) - 1 - (EndBit
> >> +
> >>  /**
> >>Worker function that returns a bit field from Operand.
> >>
> >> @@ -34,11 +42,9 @@ InternalBaseLibBitFieldReadUint (
> >>IN  UINTN 

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

Hi All

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:29 PM
> To: Udit Kumar <udit.ku...@nxp.com>; Pankaj Bansal
> <pankaj.ban...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 11:39, Udit Kumar wrote:
> >
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Friday, February 23, 2018 2:51 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
> >> <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support for Big Endian Mmio APIs
> >>
> >> On 02/23/18 09:40, Pankaj Bansal wrote:
> >>> Hi All
> >>>
> >>>> -Original Message-
> >>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >>>> Of Laszlo Ersek
> >>>> Sent: Thursday, February 22, 2018 7:26 PM
> >>>> To: Leif Lindholm <leif.lindh...@linaro.org>
> >>>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >>>> ard.biesheu...@linaro.org
> >>>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support
> >>>> for Big Endian Mmio APIs
> >>>>
> >>>> On 02/22/18 12:52, Leif Lindholm wrote:
> >>>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>>>
> >>>>>>> But that brings back the complication as to how we have a driver
> >>>>>>> that needs an LE IO library to write output, and a BE IO library
> >>>>>>> to manipulate the hardware.
> >>>>>>
> >>>>>> Can you please explain the "write output" use case more precisely?
> >>>>>>
> >>>>>> My thinking would be this:
> >>>>>>
> >>>>>> - Use the IoLib class directly for "writing output" in little
> >>>>>> endian byte order (which is still unclear to me sorry).
> >>>>>
> >>>>> If the IoLib class is mapped to a an instance that byte-swaps
> >>>>> (hereto referred to as BeIoLib if IoLibSwap is unsuitable), would
> >>>>> we not then end up mapping the non-swapping, currently
> implemented
> >>>>> in BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>>>> needing to duplicated all IoLib implementation .infs to provide an
> >>>>> IoLib and a BeIoLib for each?
> >>>>>
> >>>>> It's at that point I burst an aneurysm.
> >>>>> Am I overthinking/underthinking this?
> >>>>
> >>>> We need two library classes, one for talking to LE devices and
> >>>> another to
> >> BE
> >>>> devices. These should be usable in a given module at the same time,
> >>>> as
> >> Ard
> >>>> says.
> >>>>
> >>>> Both library classes need to work on both LE and BE CPUs (working
> >>>> from
> >> your
> >>>> suggestion that UEFI might grow BE CPU support at some point).
> >>>> Whether that is implemented by dumb, separate library instances
> >> (yielding in
> >>>> total 2*2=4 library instances), or by smart,
> >>>> CPU-endianness-agnostic
> >> library
> >>>> instances (in total, 2), is a different question.
> >>>>
> >>>> Note that such "smarts" could be less than trivial to implement:
> >>>> - check CPU endianness in each library API?
> >>>> - or check in the lib constructor only, and flip some function pointers?
> >>>> - use a dynamic PCD for caching CPU endianness?
> >>>> - use a HOB for the same?
> >>>> - use a lib global variable (for caching only on the module level)?
> >>>>
> >>>> I think the solution that saves the most on the *source* code size is:
> &g

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

Hi All

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, February 22, 2018 7:26 PM
> To: Leif Lindholm 
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/22/18 12:52, Leif Lindholm wrote:
> > On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> 
> >>> But that brings back the complication as to how we have a driver
> >>> that needs an LE IO library to write output, and a BE IO library to
> >>> manipulate the hardware.
> >>
> >> Can you please explain the "write output" use case more precisely?
> >>
> >> My thinking would be this:
> >>
> >> - Use the IoLib class directly for "writing output" in little endian
> >> byte order (which is still unclear to me sorry).
> >
> > If the IoLib class is mapped to a an instance that byte-swaps (hereto
> > referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> > end up mapping the non-swapping, currently implemented in
> > BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> > needing to duplicated all IoLib implementation .infs to provide an
> > IoLib and a BeIoLib for each?
> >
> > It's at that point I burst an aneurysm.
> > Am I overthinking/underthinking this?
> 
> We need two library classes, one for talking to LE devices and another to BE
> devices. These should be usable in a given module at the same time, as Ard
> says.
> 
> Both library classes need to work on both LE and BE CPUs (working from your
> suggestion that UEFI might grow BE CPU support at some point).
> Whether that is implemented by dumb, separate library instances (yielding in
> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic library
> instances (in total, 2), is a different question.
> 
> Note that such "smarts" could be less than trivial to implement:
> - check CPU endianness in each library API?
> - or check in the lib constructor only, and flip some function pointers?
> - use a dynamic PCD for caching CPU endianness?
> - use a HOB for the same?
> - use a lib global variable (for caching only on the module level)?
> 
> I think the solution that saves the most on the *source* code size is:
> - introduce the BeIoLib class
> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>   BeIoLib instance that we introduce
> - modify the MMIO functions in *both* lib instances (original LE, and
>   new BE), like this:
> 
>   - If the CPU architecture is known to be bound to a single endianness,
> then hardcode the appropriate operation. This can be done with
> preprocessor macros, or with the architecture support of INF files /
> separate source files. For example, on IA32 and X64, the IoLib
> instance should work transparently, unconditionally, and the BeIoLib
> instance should byte-swap, unconditionally.
> 
>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> *both* lib instances should do something like this:
> 
> //
> // at file scope
> //
> STATIC CONST UINT16 mOne = 1;
> 
> //
> // at function scope
> //
> if (*(CONST UINT8 *) == 1) {
>   //
>   // CPU in LE mode:
>   // - work transparently in the IoLib instance
>   // - byte-swap in the BeIoLib instance
>   //
> } else {
>   //
>   // CPU in BE mode:
>   // - byte-swap in the IoLib instance
>   // - work transparently in the BeIoLib instance
>   //
> }

I suggest this approach :

1. Add BeMmio* functions in existing IoLib. BeMmio* functions will swap the 
input before write and swap output after read and so on.
Mmio* functions will not perform any byte swapping
2. create second instance (a copy) of this IoLib for CPUs that are Big Endian. 
We can call it BigEndianIoLib.
 In this library Mmio* functions will swap the input before write and swap 
output after read and so on.
 BeMmio* functions will not perform any byte swapping.
3. Include the instance of IoLib in dsc file based on cpu endianness that the 
platform wants to use. i.e.
If BIG_ENDIAN == FALSE
   IoLib | ..\..\..\IoLib
   Else
  IoLib | ..\..\..\BigEndianIoLib
4. The devices that are Big endian in platform will always call BeMmio* 
functions. They need not check CPU endianness.
5. The devices that are Little endian in platform will always call Mmio* 
functions. They need not check CPU endianness.

> 
> Thanks,
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists
> .01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel=02%7C01%7Cpankaj.bansal%40nxp.com%7C930d96bb226d4491
> df2d08d579fc1717%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 36549046015230218=OtqNxwO10GcZw1GgvSdocBCwNFuuO2Am23eN
> 

Re: [edk2] [PATCH 1/2] Silicon/NXP: Add Modified SPI protocol stack

[Pankaj Bansal]

Hi everybody,

Any review comments on this patch set ?

> -Original Message-
> From: Pankaj Bansal
> Sent: Friday, February 09, 2018 5:24 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Leif Lindholm <leif.lindh...@linaro.org>;
> Michael D Kinney <michael.d.kin...@intel.com>
> Subject: [PATCH 1/2] Silicon/NXP: Add Modified SPI protocol stack
> 
> The PI 1.6 SPI specs are not adequate to handle all type of SPI
> communication, specially the QUAD mode read/write comminications with
> the periphrals that support it.
> 
> Therefore we are modifying the SPI protocol defined in PI 1.6 spec.
> 
> Untill these changes are incorporated in PI specs, we are calling it revised 
> PI
> 1.6 spec.
> 
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> ---
>  Silicon/NXP/Include/Pi/PiSpi.h   | 233 
>  .../NXP/Include/Protocol/SpiConfiguration.h  | 313 +
>  Silicon/NXP/Include/Protocol/SpiHc.h | 208 +++
>  Silicon/NXP/Include/Protocol/SpiIo.h | 168 +
>  Silicon/NXP/Include/Protocol/SpiNorFlash.h   | 269 ++
>  5 files changed, 1191 insertions(+)
> 
> diff --git a/Silicon/NXP/Include/Pi/PiSpi.h b/Silicon/NXP/Include/Pi/PiSpi.h
> new file mode 100644 index 000..8c35d86
> --- /dev/null
> +++ b/Silicon/NXP/Include/Pi/PiSpi.h
> @@ -0,0 +1,233 @@
> +/** @file
> +  Include file matches things in PI.
> +
> +  Copyright (c) 2017, Intel Corporation. All rights reserved.
> + Copyright 2017-2018 NXP
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD  License which
> + accompanies this distribution. The full text of the license may  be
> + found at http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +  @par Revision Reference:
> +This Protocol was introduced in revised UEFI PI Specification 1.6.
> +
> +**/
> +
> +#ifndef __PI_SPI_H__
> +#define __PI_SPI_H__
> +
> +///
> +/// Define the SPI flags
> +///
> +
> +/// The SPI peripheral/controller supports only half duplex transactions
> +#define SPI_HALF_DUPLEX   BIT0
> +/// The SPI peripheral/controller supports write only transactions.
> +#define SPI_SUPPORTS_WRITE_ONLY_OPERATIONSBIT1
> +/// The SPI peripheral/controller supports Read only transactions.
> +#define SPI_SUPPORTS_READ_ONLY_OPERATIONS BIT2
> +/// The SPI peripheral/controller supports Double Transfer Rate (DTR).
> +/// DTR : Transfer may be input or output on both the /// rising and
> +falling edges of the clock.
> +#define SPI_SUPPORTS_DTR_OPERATIONS   BIT3
> +/// The SPI peripheral/controller supports a 2-bit data bus
> +#define SPI_SUPPORTS_2_BIT_DATA_BUS_WIDTH BIT4
> +/// The SPI peripheral/controller supports a 4-bit data bus
> +#define SPI_SUPPORTS_4_BIT_DATA_BUS_WIDTH BIT5
> +/// The SPI peripheral/controller supports a 8-bit data bus
> +#define SPI_SUPPORTS_8_BIT_DATA_BUS_WIDTH BIT6
> +/// Transfer size includes the opcode byte
> +#define SPI_TRANSFER_SIZE_INCLUDES_OPCODE BIT7
> +/// Transfer size includes the 4 address bytes
> +#define SPI_TRANSFER_SIZE_INCLUDES_ADDRESSBIT8
> +
> +///
> +/// SPI Frame Size supported Mask
> +///
> +#define  SPI_FRAME_MASK(FrameSize)   (1U << FrameSize)
> +
> +///
> +/// Calculate the Clock cycles from number of bytes and BusWidth ///
> +#define SPI_BYTES_TO_CYCLES(Bytes, BusWidth) ( ( (Bytes << sizeof
> (UINT8)) + BusWidth - 1) / BusWidth)
> +///
> +/// Calculate the number of bytes from Clock cycles and BusWidth ///
> +#define SPI_CYCLES_TO_BYTES(Cycles, BusWidth)( (Cycles * BusWidth) >>
> sizeof (UINT8))
> +
> +///
> +/// SPI Device Path can be used to describe the device path of both SPI
> +controller /// and SPI Peripheral.
> +///
> +typedef struct {
> +  ///
> +  /// Vendor device path specifying Vendor GUID for SPI Host controller or
> SPI Peripheral.
> +  ///
> +  VENDOR_DEVICE_PATHVendor;
> +  ///
> +  /// Controller device path to distinguish between two instances of SPI
> controller or SPI Peripheral.
> +  ///
> +

Re: [edk2] [RFC] MdePkg/BaseLib: Change BitField functions.

[Pankaj Bansal]

Hi everybody,

Any comments on this change ?

> -Original Message-
> From: Pankaj Bansal
> Sent: Friday, February 09, 2018 4:30 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>; Michael D Kinney
> <michael.d.kin...@intel.com>; Liming Gao <liming@intel.com>
> Subject: [RFC] MdePkg/BaseLib: Change BitField functions.
> 
> The bit field functions in MdePkg are not working as expected.
> The restrictions on Value parameter such that Value should not be greater
> than the bitmask value range specified by StartBit and EndBit doesn't seem to
> make sense.
> 
> Also the restriction on End bit to not be equal to start bit prohibits single 
> bit
> change.
> 
> This is an attempt to correct these limitations.
> 
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Liming Gao <liming....@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> ---
> 
> Notes:
> This is a RFC patch.
> 
>  MdePkg/Library/BaseLib/BitField.c | 179 ++--
>  1 file changed, 89 insertions(+), 90 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/BitField.c
> b/MdePkg/Library/BaseLib/BitField.c
> index eb9e276..2b5be52 100644
> --- a/MdePkg/Library/BaseLib/BitField.c
> +++ b/MdePkg/Library/BaseLib/BitField.c
> @@ -2,6 +2,8 @@
>Bit field functions of BaseLib.
> 
>Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
> +  Copyright 2018 NXP
> +
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at @@ -14,6 +16,12 @@
> 
>  #include "BaseLibInternals.h"
> 
> +#define BITMASK_UNITN(StartBit, EndBit) \
> +(((MAX_UINTN) << (StartBit)) & (MAX_UINTN >> ((sizeof (UINTN) * 8)
> +- 1 - (EndBit
> +
> +#define BITMASK_UNIT64(StartBit, EndBit) \
> +(((MAX_UINT64) << (StartBit)) & (MAX_UINT64 >> ((sizeof (UINT64) *
> +8) - 1 - (EndBit
> +
>  /**
>Worker function that returns a bit field from Operand.
> 
> @@ -34,11 +42,9 @@ InternalBaseLibBitFieldReadUint (
>IN  UINTN EndBit
>)
>  {
> -  //
> -  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
> -  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
> -  //
> -  return (Operand & ~((UINTN)-2 << EndBit)) >> StartBit;
> +  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
> +
> +  return ( (Operand & Mask) >> StartBit);
>  }
> 
>  /**
> @@ -68,19 +74,9 @@ InternalBaseLibBitFieldOrUint (
>IN  UINTN OrData
>)
>  {
> -  //
> -  // Higher bits in OrData those are not used must be zero.
> -  //
> -  // EndBit - StartBit + 1 might be 32 while the result right shifting 32 on 
> a
> 32bit integer is undefined,
> -  // So the logic is updated to right shift (EndBit - StartBit) bits and 
> compare
> the last bit directly.
> -  //
> -  ASSERT ((OrData >> (EndBit - StartBit)) == ((OrData >> (EndBit - 
> StartBit)) &
> 1));
> -
> -  //
> -  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
> -  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
> -  //
> -  return Operand | ((OrData << StartBit) & ~((UINTN) -2 << EndBit));
> +  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
> +
> +  return ( ( (Operand | OrData) & Mask) | (Operand & ~Mask));
>  }
> 
>  /**
> @@ -110,19 +106,38 @@ InternalBaseLibBitFieldAndUint (
>IN  UINTN AndData
>)
>  {
> -  //
> -  // Higher bits in AndData those are not used must be zero.
> -  //
> -  // EndBit - StartBit + 1 might be 32 while the result right shifting 32 on 
> a
> 32bit integer is undefined,
> -  // So the logic is updated to right shift (EndBit - StartBit) bits and 
> compare
> the last bit directly.
> -  //
> -  ASSERT ((AndData >> (EndBit - StartBit)) == ((AndData >> (EndBit - 
> StartBit))
> & 1));
> -
> -  //
> -  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
> -  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
> -  //
> -  return Operand & ~((~AndData << StartBit) & ~((UINTN)-2 << EndBit));
> +  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
> +
> +  return  ( ( (Operand & AndData) & Mask) | (Operand & ~Mask))

[edk2] [PATCH 1/2] Silicon/NXP: Add Modified SPI protocol stack

[Pankaj Bansal]

The PI 1.6 SPI specs are not adequate to handle all type of SPI
communication, specially the QUAD mode read/write comminications
with the periphrals that support it.

Therefore we are modifying the SPI protocol defined in PI 1.6 spec.

Untill these changes are incorporated in PI specs, we are calling it
revised PI 1.6 spec.

Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
 Silicon/NXP/Include/Pi/PiSpi.h   | 233 
 .../NXP/Include/Protocol/SpiConfiguration.h  | 313 +
 Silicon/NXP/Include/Protocol/SpiHc.h | 208 +++
 Silicon/NXP/Include/Protocol/SpiIo.h | 168 +
 Silicon/NXP/Include/Protocol/SpiNorFlash.h   | 269 ++
 5 files changed, 1191 insertions(+)

diff --git a/Silicon/NXP/Include/Pi/PiSpi.h b/Silicon/NXP/Include/Pi/PiSpi.h
new file mode 100644
index 000..8c35d86
--- /dev/null
+++ b/Silicon/NXP/Include/Pi/PiSpi.h
@@ -0,0 +1,233 @@
+/** @file
+  Include file matches things in PI.
+
+  Copyright (c) 2017, Intel Corporation. All rights reserved.
+  Copyright 2017-2018 NXP
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD
+  License which accompanies this distribution. The full text of the license may
+  be found at http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+  @par Revision Reference:
+This Protocol was introduced in revised UEFI PI Specification 1.6.
+
+**/
+
+#ifndef __PI_SPI_H__
+#define __PI_SPI_H__
+
+///
+/// Define the SPI flags
+///
+
+/// The SPI peripheral/controller supports only half duplex transactions
+#define SPI_HALF_DUPLEX   BIT0
+/// The SPI peripheral/controller supports write only transactions.
+#define SPI_SUPPORTS_WRITE_ONLY_OPERATIONSBIT1
+/// The SPI peripheral/controller supports Read only transactions.
+#define SPI_SUPPORTS_READ_ONLY_OPERATIONS BIT2
+/// The SPI peripheral/controller supports Double Transfer Rate (DTR).
+/// DTR : Transfer may be input or output on both the
+/// rising and falling edges of the clock.
+#define SPI_SUPPORTS_DTR_OPERATIONS   BIT3
+/// The SPI peripheral/controller supports a 2-bit data bus
+#define SPI_SUPPORTS_2_BIT_DATA_BUS_WIDTH BIT4
+/// The SPI peripheral/controller supports a 4-bit data bus
+#define SPI_SUPPORTS_4_BIT_DATA_BUS_WIDTH BIT5
+/// The SPI peripheral/controller supports a 8-bit data bus
+#define SPI_SUPPORTS_8_BIT_DATA_BUS_WIDTH BIT6
+/// Transfer size includes the opcode byte
+#define SPI_TRANSFER_SIZE_INCLUDES_OPCODE BIT7
+/// Transfer size includes the 4 address bytes
+#define SPI_TRANSFER_SIZE_INCLUDES_ADDRESSBIT8
+
+///
+/// SPI Frame Size supported Mask
+///
+#define  SPI_FRAME_MASK(FrameSize)   (1U << FrameSize)
+
+///
+/// Calculate the Clock cycles from number of bytes and BusWidth
+///
+#define SPI_BYTES_TO_CYCLES(Bytes, BusWidth) ( ( (Bytes << sizeof (UINT8)) 
+ BusWidth - 1) / BusWidth)
+///
+/// Calculate the number of bytes from Clock cycles and BusWidth
+///
+#define SPI_CYCLES_TO_BYTES(Cycles, BusWidth)( (Cycles * BusWidth) >> 
sizeof (UINT8))
+
+///
+/// SPI Device Path can be used to describe the device path of both SPI 
controller
+/// and SPI Peripheral.
+///
+typedef struct {
+  ///
+  /// Vendor device path specifying Vendor GUID for SPI Host controller or SPI 
Peripheral.
+  ///
+  VENDOR_DEVICE_PATHVendor;
+  ///
+  /// Controller device path to distinguish between two instances of SPI 
controller or SPI Peripheral.
+  ///
+  CONTROLLER_DEVICE_PATHController;
+  ///
+  /// Signify the end of Device Path.
+  ///
+  EFI_DEVICE_PATH_PROTOCOL  End;
+} EFI_SPI_DEVICE_PATH;
+
+///
+/// Note: The revised UEFI PI 1.6 specification does not specify values for the
+///   members below. The order matches the specification.
+///
+typedef enum {
+  ///
+  /// Data flowing from the host to the SPI peripheral
+  /// or Data flowing from the SPI peripheral to the host
+  ///
+  SPI_TRANSACTION_DATA = 0,
+
+  ///
+  /// Command to send to SPI Peripheral
+  ///
+  SPI_TRANSACTION_COMMAND,
+
+  ///
+  /// Offset in SPI Peripheral from/to which data is to be read/written
+  ///
+  SPI_TRANSACTION_ADDRESS,
+
+  ///
+  /// Optional control bits that follow the address bits.
+  /// These bits are driven by the controller if they are specified.
+  ///
+  /// NOTE This field should be counted in clocks not number of bits received 
by the
+  /// serial flash. The SPI master drives the bus during "mode bits" cycle

[edk2] [PATCH 2/2] NXP/SpiBusDxe: Add SPI Bus driver.

[Pankaj Bansal]

This Driver is based on revised PI 1.6 SPI specs.

This driver is DXE_RUNTIME_DRIVER, so that the SPI peripherals that
are needed to support runtime services can be used with this driver.

This driver follows UEFI driver model and its a Bus Driver that creates
all of its child handles on the first call to Start.

Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
 Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.c   | 1548 +
 Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.h   |  510 ++
 Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.inf |   53 +
 3 files changed, 2111 insertions(+)

diff --git a/Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.c 
b/Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.c
new file mode 100644
index 000..70c0aba
--- /dev/null
+++ b/Silicon/NXP/Drivers/SpiBusDxe/SpiBusDxe.c
@@ -0,0 +1,1548 @@
+/** @file
+  This file implements SPI IO Protocol which enables the user to manipulate a 
single
+  SPI device independent of the host controller and SPI design.
+
+  Based on MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c
+
+  Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.
+  Copyright 2018 NXP
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+ @par Specification Reference:
+  - UEFI 2.7 errata A, Chapter 8, Runtime Services
+  - UEFI 2.7 errata A, Chapter 10, Device Path Protocol
+  - UEFI 2.7 errata A, Chapter 11, UEFI Driver Model
+  - PI 1.6, Volume 5, Chapter 18 SPI Protocol Stack
+**/
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "SpiBusDxe.h"
+
+//
+// Global Variables
+//
+extern EFI_COMPONENT_NAME_PROTOCOLgSpiBusComponentName;
+extern EFI_COMPONENT_NAME2_PROTOCOL   gSpiBusComponentName2;
+extern EFI_DRIVER_BINDING_PROTOCOLgSpiBusDriverBinding;
+
+//
+//  EFI_DRIVER_BINDING_PROTOCOL instance
+//
+EFI_DRIVER_BINDING_PROTOCOL gSpiBusDriverBinding = {
+  .Supported = SpiBusDriverSupported,
+  .Start = SpiBusDriverStart,
+  .Stop = SpiBusDriverStop,
+  .Version = 0x10,
+  .ImageHandle = NULL,
+  .DriverBindingHandle = NULL
+};
+
+//
+// Template for SPI Bus Context
+//
+SPI_BUS_CONTEXT gEfiSpiBusContextTemplate = {
+  .Signature = SPI_BUS_SIGNATURE,
+  .SpiHost = NULL,
+  .SpiBus = NULL,
+  .Link = {
+.ForwardLink = NULL,
+.BackLink = NULL
+  }
+};
+
+//
+// Template for SPI Device Context
+//
+SPI_DEVICE_CONTEXT gEfiSpiDeviceContextTemplate = {
+  .Signature = SPI_DEVICE_SIGNATURE,
+  .Handle = NULL,
+  .SpiIo = {
+.SpiPeripheral = NULL,
+.OriginalSpiPeripheral = NULL,
+.FrameSizeSupportMask = 0,
+.MaximumTransferBytes = 1,
+.Attributes = 0,
+.LegacySpiProtocol = NULL,
+.Transaction = SpiBusTransaction,
+.UpdateSpiPeripheral = SpiBusUpdateSpiPeripheral
+  },
+  .SpiBusContext = NULL,
+  .Link = {
+.ForwardLink = NULL,
+.BackLink = NULL
+  }
+};
+
+STATIC EFI_EVENT mSpiBusVirtualAddrChangeEvent;
+
+// Link list of SPI Buses that are runtime
+STATIC LIST_ENTRYmSpiBusList = INITIALIZE_LIST_HEAD_VARIABLE (mSpiBusList);
+
+// Link list of SPI Devices that are runtime
+STATIC LIST_ENTRYmSpiDeviceList = INITIALIZE_LIST_HEAD_VARIABLE 
(mSpiDeviceList);
+
+//
+// Driver name table
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_UNICODE_STRING_TABLE 
mSpiBusDriverNameTable[] = {
+  { "eng;en", (CHAR16 *) L"SPI Bus Driver" },
+  { NULL , NULL }
+};
+
+//
+// EFI Component Name Protocol
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME_PROTOCOL  
gSpiBusComponentName = {
+  (EFI_COMPONENT_NAME_GET_DRIVER_NAME) SpiBusComponentNameGetDriverName,
+  (EFI_COMPONENT_NAME_GET_CONTROLLER_NAME) 
SpiBusComponentNameGetControllerName,
+  "eng"
+};
+
+//
+// EFI Component Name 2 Protocol
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME2_PROTOCOL 
gSpiBusComponentName2 = {
+  SpiBusComponentNameGetDriverName,
+  SpiBusComponentNameGetControllerName,
+  "en"
+};
+
+/**
+  Retrieves a Unicode string that is the user readable name of the driver.
+
+  This function retrieves the user readable name of a driver in the form of a
+  Unicode string. If the driver specified by This has a user readable name in
+  the language specified by Language, then a pointer to the driver name is
+  returned in DriverName, and EFI_SUCCESS is returned. If the driver specified
+  by This does not support the language specified by Languag

[edk2] [RFC] MdePkg/BaseLib: Change BitField functions.

[Pankaj Bansal]

The bit field functions in MdePkg are not working as expected.
The restrictions on Value parameter such that Value should not be
greater than the bitmask value range specified by StartBit and EndBit
doesn't seem to make sense.

Also the restriction on End bit to not be equal to start bit prohibits
single bit change.

This is an attempt to correct these limitations.

Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <liming@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---

Notes:
This is a RFC patch.

 MdePkg/Library/BaseLib/BitField.c | 179 ++--
 1 file changed, 89 insertions(+), 90 deletions(-)

diff --git a/MdePkg/Library/BaseLib/BitField.c 
b/MdePkg/Library/BaseLib/BitField.c
index eb9e276..2b5be52 100644
--- a/MdePkg/Library/BaseLib/BitField.c
+++ b/MdePkg/Library/BaseLib/BitField.c
@@ -2,6 +2,8 @@
   Bit field functions of BaseLib.
 
   Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
+  Copyright 2018 NXP
+
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -14,6 +16,12 @@
 
 #include "BaseLibInternals.h"
 
+#define BITMASK_UNITN(StartBit, EndBit) \
+(((MAX_UINTN) << (StartBit)) & (MAX_UINTN >> ((sizeof (UINTN) * 8) - 1 - 
(EndBit
+
+#define BITMASK_UNIT64(StartBit, EndBit) \
+(((MAX_UINT64) << (StartBit)) & (MAX_UINT64 >> ((sizeof (UINT64) * 8) - 1 
- (EndBit
+
 /**
   Worker function that returns a bit field from Operand.
 
@@ -34,11 +42,9 @@ InternalBaseLibBitFieldReadUint (
   IN  UINTN EndBit
   )
 {
-  //
-  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
-  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
-  //
-  return (Operand & ~((UINTN)-2 << EndBit)) >> StartBit;
+  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
+
+  return ( (Operand & Mask) >> StartBit);
 }
 
 /**
@@ -68,19 +74,9 @@ InternalBaseLibBitFieldOrUint (
   IN  UINTN OrData
   )
 {
-  //
-  // Higher bits in OrData those are not used must be zero. 
-  //
-  // EndBit - StartBit + 1 might be 32 while the result right shifting 32 on a 
32bit integer is undefined,
-  // So the logic is updated to right shift (EndBit - StartBit) bits and 
compare the last bit directly.
-  //
-  ASSERT ((OrData >> (EndBit - StartBit)) == ((OrData >> (EndBit - StartBit)) 
& 1));
-  
-  //
-  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
-  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
-  //
-  return Operand | ((OrData << StartBit) & ~((UINTN) -2 << EndBit));
+  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
+
+  return ( ( (Operand | OrData) & Mask) | (Operand & ~Mask));
 }
 
 /**
@@ -110,19 +106,38 @@ InternalBaseLibBitFieldAndUint (
   IN  UINTN AndData
   )
 {
-  //
-  // Higher bits in AndData those are not used must be zero. 
-  //
-  // EndBit - StartBit + 1 might be 32 while the result right shifting 32 on a 
32bit integer is undefined,
-  // So the logic is updated to right shift (EndBit - StartBit) bits and 
compare the last bit directly.
-  //
-  ASSERT ((AndData >> (EndBit - StartBit)) == ((AndData >> (EndBit - 
StartBit)) & 1));
-
-  //
-  // ~((UINTN)-2 << EndBit) is a mask in which bit[0] thru bit[EndBit]
-  // are 1's while bit[EndBit + 1] thru the most significant bit are 0's.
-  //
-  return Operand & ~((~AndData << StartBit) & ~((UINTN)-2 << EndBit));
+  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
+
+  return  ( ( (Operand & AndData) & Mask) | (Operand & ~Mask));
+}
+
+/**
+  Worker function that writes a bit field to an value, and returns the result.
+
+  Writes Value to the bit field specified by the StartBit and the EndBit in
+  Operand. All other bits in Operand are preserved. The new 8-bit value is
+  returned.
+
+  @param  Operand   Operand on which to perform the bitfield operation.
+  @param  StartBit  The ordinal of the least significant bit in the bit field.
+  @param  EndBitThe ordinal of the most significant bit in the bit field.
+  @param  Value The new value of the bit field.
+
+  @return The new value.
+
+**/
+UINTN
+EFIAPI
+InternalBaseLibBitFieldWriteUint (
+  IN  UINTN Operand,
+  IN  UINTN StartBit,
+  IN  UINTN EndBit,
+  IN  UINTN Value
+  )
+{
+  UINTN  Mask = BITMASK_UNITN (StartBit, EndBit);
+
+  return  ( (Value & Mask) | (Operand & ~Mask));
 }
 
 /**
@@ -153,7 +168,7 @@ BitFieldRead8 (
   )
 {
   A

Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 10, 2018 10:34 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> GetElapsedTime function of TimerLib
> 
> Hi Pankaj,
> 
> On 01/10/18 17:05, Pankaj Bansal wrote:
> > Hi Laszlo,
> >
> > Thanks for reviewing my changes and thanks for pointing me to the thread
> that you worked on.
> > It was really helpful. please see my response inline ..
> >
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Wednesday, January 10, 2018 7:46 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> >> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> >> <ard.biesheu...@linaro.org>; Michael Kinney
> >> <michael.d.kin...@intel.com>
> >> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> >> GetElapsedTime function of TimerLib
> >>
> >> Hello Pankaj,
> >>
> >> On 01/10/18 10:31, Pankaj Bansal wrote:
> >>> This function calculates the time elaped in Naoseconds between call
> >>> to this function and BaseTime, which is passed as argument.
> >>>
> >>> This is particularly useful in detecting timeout conditions.
> >>>
> >>> Cc: Leif Lindholm <leif.lindh...@linaro.org>
> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> >>>
> >>> diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> index b81293c..0898339 100644
> >>> --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> >>> @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
> >>>
> >>>return NanoSeconds;
> >>>  }
> >>> +
> >>> +/**
> >>> +  Get Elapsed time in Nanoseonds w.r.t BaseTime
> >>> +
> >>> +  This function calculates the time elaped in Naoseconds between
> >>> + call to this  function and BaseTime, which is passed as argument.
> >>> +
> >>> +  @param  BaseTime BaseTime in NanoSeconds.
> >>> +
> >>> +  @return The elapsed time in nanoseconds.
> >>> +
> >>> +**/
> >>> +UINT64
> >>> +EFIAPI
> >>> +GetElapsedTime (
> >>> +  IN  UINT64 BaseTime
> >>> +  )
> >>> +{
> >>> +  UINT64  NanoSeconds;
> >>> +  UINT64  Ticks;
> >>> +
> >>> +  //
> >>> +  // Get current Ticks.
> >>> +  //
> >>> +  Ticks = GetPerformanceCounter();
> >>> +
> >>> +  //
> >>> +  // Convert Ticks to Nanoseconds
> >>> +  //
> >>> +  NanoSeconds = GetTimeInNanoSecond (Ticks);
> >>> +
> >>> +  //
> >>> +  // return the difference
> >>> +  //
> >>> +  return (NanoSeconds - BaseTime);
> >>> +}
> >>>
> >>
> >> There are two problems with this patch set:
> >>
> >> (1) The TimerLib.h file (in the first patch) is a public library
> >> class header, for which several library instances (implementations)
> >> exist. So, introducing a new API requires adding an implementation
> >> (the same implementation, or different ones, as necessary) to *all*
> instances in the edk2 tree.
> >
> > I agree with your intent to prevent code duplication. But I still feel that 
> > we
> should enhance TimerLib.
> > Because, a user intending to use various timer functionalities, should
> include only one header file (TimerLib.h) and one Library.
> > Like Mike pointed out in your thread to enhance the TimerTickDiffLib,
> > in future if someone wishes to add some new functionality related to
> > timer and if he writes a new library for it, then its added cumbersome
> > for users (user has to include 3 header files and libraries 3.)
> >
> > To prevent code duplication, can we look at other ways like

Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

Hi Laszlo,

Thanks for reviewing my changes and thanks for pointing me to the thread that 
you worked on.
It was really helpful. please see my response inline ..

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 10, 2018 7:46 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Michael Kinney <michael.d.kin...@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement
> GetElapsedTime function of TimerLib
> 
> Hello Pankaj,
> 
> On 01/10/18 10:31, Pankaj Bansal wrote:
> > This function calculates the time elaped in Naoseconds between call to
> > this function and BaseTime, which is passed as argument.
> >
> > This is particularly useful in detecting timeout conditions.
> >
> > Cc: Leif Lindholm <leif.lindh...@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> >
> > diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > index b81293c..0898339 100644
> > --- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > +++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
> > @@ -290,3 +290,39 @@ GetTimeInNanoSecond (
> >
> >return NanoSeconds;
> >  }
> > +
> > +/**
> > +  Get Elapsed time in Nanoseonds w.r.t BaseTime
> > +
> > +  This function calculates the time elaped in Naoseconds between call
> > + to this  function and BaseTime, which is passed as argument.
> > +
> > +  @param  BaseTime BaseTime in NanoSeconds.
> > +
> > +  @return The elapsed time in nanoseconds.
> > +
> > +**/
> > +UINT64
> > +EFIAPI
> > +GetElapsedTime (
> > +  IN  UINT64 BaseTime
> > +  )
> > +{
> > +  UINT64  NanoSeconds;
> > +  UINT64  Ticks;
> > +
> > +  //
> > +  // Get current Ticks.
> > +  //
> > +  Ticks = GetPerformanceCounter();
> > +
> > +  //
> > +  // Convert Ticks to Nanoseconds
> > +  //
> > +  NanoSeconds = GetTimeInNanoSecond (Ticks);
> > +
> > +  //
> > +  // return the difference
> > +  //
> > +  return (NanoSeconds - BaseTime);
> > +}
> >
> 
> There are two problems with this patch set:
> 
> (1) The TimerLib.h file (in the first patch) is a public library class 
> header, for
> which several library instances (implementations) exist. So, introducing a
> new API requires adding an implementation (the same implementation, or
> different ones, as necessary) to *all* instances in the edk2 tree.

I agree with your intent to prevent code duplication. But I still feel that we 
should enhance TimerLib.
Because, a user intending to use various timer functionalities, should include 
only one header file (TimerLib.h) and one Library.
Like Mike pointed out in your thread to enhance the TimerTickDiffLib, in future 
if someone wishes to add some new functionality related
to timer and if he writes a new library for it, then its added cumbersome for 
users (user has to include 3 header files and libraries 3.)

To prevent code duplication, can we look at other ways like using weak symbols 
for such functions that are platform agnostic.
Or putting these generic functions in one .c file in MdePkg and include that 
file in inf file of TimerLib implementation?

> 
> (2) The calculation in your GetElapsedTime() function is wrong.
> GetTimeInNanoSecond() converts a small *difference* of ticks to time. It
> does not convert an absolute tick value to an absolute time.

The GetTimeInNanoSecond() is only concerned with the ticks. Those ticks can be 
a difference of ticks or absolute ticks from counter start.
The *difference* part of GetElapsedTime would depend on its usage. Like this :
Start = GetElapsedTime (0);
// do something 
End = GetElapsedTime (Start);

> 
> Furthermore, tick differences are less trivial to calculate than one might
> imagine, because (a) a performance counter may count down, and
> *independently*, (b) the numeric low bound of the counter range may not
> be zero.
> 

This is nice observation. It was an oversight on my part.

> Earlier I proposed a new TimerTickDiffLib class (and implementation) for
> centralizing exactly this kind of calculation. Please see the thread at:
> 
>   [edk2] TimerTickDiffLib for MdePkg?
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.
> mail-archive.com%2F8cba2a58-1333-7733-031d

[edk2] [PATCH 2/2] ArmPkg/ArmArchTimerLib: Implement GetElapsedTime function of TimerLib

[Pankaj Bansal]

This function calculates the time elaped in Naoseconds between call to
this function and BaseTime, which is passed as argument.

This is particularly useful in detecting timeout conditions.

Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>

diff --git a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c 
b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
index b81293c..0898339 100644
--- a/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
+++ b/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.c
@@ -290,3 +290,39 @@ GetTimeInNanoSecond (
 
   return NanoSeconds;
 }
+
+/**
+  Get Elapsed time in Nanoseonds w.r.t BaseTime
+
+  This function calculates the time elaped in Naoseconds between call to this
+  function and BaseTime, which is passed as argument.
+
+  @param  BaseTime BaseTime in NanoSeconds.
+
+  @return The elapsed time in nanoseconds.
+
+**/
+UINT64
+EFIAPI
+GetElapsedTime (
+  IN  UINT64 BaseTime
+  )
+{
+  UINT64  NanoSeconds;
+  UINT64  Ticks;
+
+  //
+  // Get current Ticks.
+  //
+  Ticks = GetPerformanceCounter();
+
+  //
+  // Convert Ticks to Nanoseconds
+  //
+  NanoSeconds = GetTimeInNanoSecond (Ticks);
+
+  //
+  // return the difference
+  //
+  return (NanoSeconds - BaseTime);
+}
-- 
2.7.4

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

[edk2] [PATCH 1/2] MdePkg/TimerLib: Add a function to calculate elapsed time

[Pankaj Bansal]

This function calculates the time elaped in Naoseconds between call to
this function and BaseTime, which is passed as argument.

This is particularly useful in detecting timeout conditions.

Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <liming@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>

diff --git a/MdePkg/Include/Library/TimerLib.h 
b/MdePkg/Include/Library/TimerLib.h
index ecc3ad3..82a5c5c 100644
--- a/MdePkg/Include/Library/TimerLib.h
+++ b/MdePkg/Include/Library/TimerLib.h
@@ -111,4 +111,20 @@ GetTimeInNanoSecond (
   IN  UINT64 Ticks
   );
 
+/**
+  Get Elapsed time in Nanoseonds w.r.t BaseTime
+
+  This function calculates the time elaped in Naoseconds between call to this
+  function and BaseTime, which is passed as argument.
+
+  @param  BaseTime BaseTime in NanoSeconds.
+
+  @return The elapsed time in nanoseconds.
+
+**/
+UINT64
+EFIAPI
+GetElapsedTime (
+  IN  UINT64 BaseTime
+  );
 #endif
-- 
2.7.4

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

[edk2] [PATCH] EmbeddedPkg/FdtLib: Update FdtLib to v1.4.5

[Pankaj Bansal]

Update the FdtLib so that new APIs provided by FdtLib like
fdt_address_cells, fdt_size_cells etc. can be used.

Reference code:
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt?h=v1.4.5

Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---

Notes:
The source code has been copied under BSD license terms.
One file fdt_overlay.c which is part of libfdt has no license
header specified at the top of file.
Therefore that file and the references to functions defined in
that file have been removed.

 EmbeddedPkg/Include/libfdt.h | 416 +++--
 EmbeddedPkg/Include/libfdt_env.h |   4 +
 EmbeddedPkg/Library/FdtLib/FdtLib.inf|   1 +
 EmbeddedPkg/Library/FdtLib/Makefile.libfdt   |   3 +-
 EmbeddedPkg/Library/FdtLib/fdt.c |  13 +-
 EmbeddedPkg/Library/FdtLib/fdt_addresses.c   |  96 
 EmbeddedPkg/Library/FdtLib/fdt_empty_tree.c  |   1 -
 EmbeddedPkg/Library/FdtLib/fdt_ro.c  | 163 ++-
 EmbeddedPkg/Library/FdtLib/fdt_rw.c  |  50 +-
 EmbeddedPkg/Library/FdtLib/fdt_strerror.c|   6 +
 EmbeddedPkg/Library/FdtLib/fdt_sw.c  |  48 +-
 EmbeddedPkg/Library/FdtLib/fdt_wip.c |  33 +-
 EmbeddedPkg/Library/FdtLib/libfdt_internal.h |   6 +-
 EmbeddedPkg/Library/FdtLib/version.lds   |   7 +
 14 files changed, 760 insertions(+), 87 deletions(-)

diff --git a/EmbeddedPkg/Include/libfdt.h b/EmbeddedPkg/Include/libfdt.h
index 212cb99..1aabec3 100644
--- a/EmbeddedPkg/Include/libfdt.h
+++ b/EmbeddedPkg/Include/libfdt.h
@@ -61,7 +61,7 @@
 #define FDT_ERR_NOTFOUND   1
/* FDT_ERR_NOTFOUND: The requested node or property does not exist */
 #define FDT_ERR_EXISTS 2
-   /* FDT_ERR_EXISTS: Attemped to create a node or property which
+   /* FDT_ERR_EXISTS: Attempted to create a node or property which
 * already exists */
 #define FDT_ERR_NOSPACE3
/* FDT_ERR_NOSPACE: Operation needed to expand the device
@@ -79,8 +79,10 @@
 * (e.g. missing a leading / for a function which requires an
 * absolute path) */
 #define FDT_ERR_BADPHANDLE 6
-   /* FDT_ERR_BADPHANDLE: Function was passed an invalid phandle
-* value.  phandle values of 0 and -1 are not permitted. */
+   /* FDT_ERR_BADPHANDLE: Function was passed an invalid phandle.
+* This can be caused either by an invalid phandle property
+* length, or the phandle value was either 0 or -1, which are
+* not permitted. */
 #define FDT_ERR_BADSTATE   7
/* FDT_ERR_BADSTATE: Function was passed an incomplete device
 * tree created by the sequential-write functions, which is
@@ -116,13 +118,34 @@
 * Should never be returned, if it is, it indicates a bug in
 * libfdt itself. */
 
-#define FDT_ERR_MAX13
+/* Errors in device tree content */
+#define FDT_ERR_BADNCELLS  14
+   /* FDT_ERR_BADNCELLS: Device tree has a #address-cells, #size-cells
+* or similar property with a bad format or value */
+
+#define FDT_ERR_BADVALUE   15
+   /* FDT_ERR_BADVALUE: Device tree has a property with an unexpected
+* value. For example: a property expected to contain a string list
+* is not NUL-terminated within the length of its value. */
+
+#define FDT_ERR_BADOVERLAY 16
+   /* FDT_ERR_BADOVERLAY: The device tree overlay, while
+* correctly structured, cannot be applied due to some
+* unexpected or missing value, property or node. */
+
+#define FDT_ERR_NOPHANDLES 17
+   /* FDT_ERR_NOPHANDLES: The device tree doesn't have any
+* phandle available anymore without causing an overflow */
+
+#define FDT_ERR_MAX17
 
 /**/
 /* Low-level functions (you probably don't need these)*/
 /**/
 
+#ifndef SWIG /* This function is not useful in Python */
 const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
+#endif
 static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
 {
return (void *)(uintptr_t)fdt_offset_ptr(fdt, offset, checklen);
@@ -158,27 +181,54 @@ int fdt_first_subnode(const void *fdt, int offset);
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
+/**
+ * fdt_for_each_subnode - iterate over all subnodes of a parent
+ *
+ * @node:  child node (int, lvalue)
+ * @fdt:   FDT blob (const void *)
+ * @parent:parent node (int)
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ * fdt_for_each_subnode(node, fdt, parent) {
+ * Use node
+ * ...
+ * }
+ *
+ * if ((node < 0) &&

[edk2] [RFC] MdePkg/Spi : Modify SPI Protocol strctures to make it more generic

[Pankaj Bansal]

Issue_1: Currenty SPI IO and SPI HC protocol strcture is not equipped
to handle all type of SPI commands
e.g. QuadIo Read Command (0xEBh) in spansion flash S25FS512S will not
work.

Cause: Currenty SPI protocol describes a SPI transaction as combination
of data and address and command, which would all be transmitted on
same data bus (1 or 2 or 4).

BUT when we have a case in which command is send one single data bus and
address and data is sent one 4 data bus (like 0xEB in spansion), then
this will not work with current strcture.

Fix: Instead of making a single transaction, we can specify a group of
transactions (called packet, terminology borrowed from I2c Protocol) in
which each transaction specifies the data width, frequency, delay and
read/write info.

Issue_2: Specify additional data about SPI transaction (i.e. transaction
is of type cmd, address, data, dummy etc). some controllers need this
info to communicate with SPI peripheral. others can ignore this info.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
 MdePkg/Include/Protocol/SpiHc.h |  15 +-
 MdePkg/Include/Protocol/SpiIo.h | 250 --
 2 files changed, 104 insertions(+), 161 deletions(-)

diff --git a/MdePkg/Include/Protocol/SpiHc.h b/MdePkg/Include/Protocol/SpiHc.h
index 12fe5d2..65964bc 100644
--- a/MdePkg/Include/Protocol/SpiHc.h
+++ b/MdePkg/Include/Protocol/SpiHc.h
@@ -110,8 +110,11 @@ typedef EFI_STATUS
   status.
 
   @param[in] ThisPointer to an EFI_SPI_HC_PROTOCOL structure.
-  @param[in] BusTransaction  Pointer to a EFI_SPI_BUS_ TRANSACTION containing
- the description of the SPI transaction to perform.
+  @param[in] SpiPeripheral   The address of an EFI_SPI_PERIPHERAL data 
structure
+ describing the SPI peripheral from(to) which
+ read(write) transactions to be performed.
+  @param[in] RequestPacket   Pointer to a EFI_SPI_REQUEST_PACKET containing
+ the description of the SPI transactions to 
perform.
 
   @retval EFI_SUCCESS  The transaction completed successfully
   @retval EFI_BAD_BUFFER_SIZE  The BusTransaction->WriteBytes value is invalid,
@@ -124,7 +127,8 @@ typedef EFI_STATUS
 typedef EFI_STATUS
 (EFIAPI *EFI_SPI_HC_PROTOCOL_TRANSACTION) (
   IN CONST EFI_SPI_HC_PROTOCOL  *This,
-  IN EFI_SPI_BUS_TRANSACTION*BusTransaction
+  IN CONST EFI_SPI_PERIPHERAL   *SpiPeripheral;
+  IN EFI_SPI_REQUEST_PACKET *RequestPacket
   );
 
 ///
@@ -135,7 +139,6 @@ struct _EFI_SPI_HC_PROTOCOL {
   /// Host control attributes, may have zero or more of the following set:
   /// * HC_SUPPORTS_WRITE_ONLY_OPERATIONS
   /// * HC_SUPPORTS_READ_ONLY_OPERATIONS
-  /// * HC_SUPPORTS_WRITE_THEN_READ_OPERATIONS
   /// * HC_TX_FRAME_IN_MOST_SIGNIFICANT_BITS
   ///   - The SPI host controller requires the transmit frame to be in most
   /// significant bits instead of least significant bits.The host driver
@@ -149,10 +152,6 @@ struct _EFI_SPI_HC_PROTOCOL {
   ///   - The SPI controller supports a 2 - bit data bus
   /// * HC_SUPPORTS_4_B1T_DATA_BUS_WIDTH
   ///   - The SPI controller supports a 4 - bit data bus
-  /// * HC_TRANSFER_SIZE_INCLUDES_OPCODE
-  ///   - Transfer size includes the opcode byte
-  /// * HC_TRANSFER_SIZE_INCLUDES_ADDRESS
-  ///   - Transfer size includes the 3 address bytes
   /// The SPI host controller must support full - duplex (receive while
   /// sending) operation.The SPI host controller must support a 1 - bit bus
   /// width.
diff --git a/MdePkg/Include/Protocol/SpiIo.h b/MdePkg/Include/Protocol/SpiIo.h
index 43e8045..507abb5 100644
--- a/MdePkg/Include/Protocol/SpiIo.h
+++ b/MdePkg/Include/Protocol/SpiIo.h
@@ -27,147 +27,33 @@ typedef struct _EFI_SPI_IO_PROTOCOL EFI_SPI_IO_PROTOCOL;
 /// Note: The UEFI PI 1.6 specification does not specify values for the
 ///   members below. The order matches the specification.
 ///
+///
+/// SPI peripheral transfer type
+///
+/// The EFI_SPI_TRANSACTION_TYPE describes the type of SPI transaction to 
either
+/// send or recievce from SPI peripheral.
+/// some SPI controllers need this information to complete a SPI operation
+/// (which consists of one or many transactions)
+///
+/// for other controllers this field may be left 0 (SPI_TRANSACTION_NONE)
+///
 typedef enum {
-  ///
-  /// Data flowing in both direction between the host and
-  /// SPI peripheral.ReadBytes must equal WriteBytes and both ReadBuffer and
-  /// WriteBuffer must be provided.
-  ///
-  SPI_TRANSACTION_FULL_DUPLEX,
-
-  ///
-  /// Data flowing from the host to the SPI peripheral.ReadBytes must be
-  /// zero.WriteBytes must be non - zero and WriteBuffer must be provided.
-  ///
-  SPI_TRANSACTION_WRITE_ONLY,
-
-  ///
-  /// Data flowing from the SPI peripheral to the host.WriteBytes must be
-  /// zero.ReadBytes must be non - zero and ReadBuffer must be

Re: [edk2] [PATCH 1/1] EmbeddedPkg: Implement NorFlashLib

[Pankaj Bansal]

Hi Marcin,

The SPI flash Jedec id is not standardized across flashes.
The JEDEDC group only assigns first byte (Manufacturer ID) to different vendors.
Rest of the bytes are up to the Manufacturer to decide.
Therefore this info should be left to the SPI flash driver to parse.
It should not be stored in tabular form rather calculated based on JEDEDC data 
read.

Thanks & Regards,
Pankaj Bansal

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Marcin 
Wojtas
Sent: Tuesday, October 31, 2017 1:46 PM
To: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Tian, Feng <feng.t...@intel.com>; Hua Jing <jing...@marvell.com>; Ard 
Biesheuvel <ard.biesheu...@linaro.org>; edk2-devel-01 
<edk2-devel@lists.01.org>; Gao, Liming <liming@intel.com>; 
nad...@marvell.com; Kostya Porotchkin <kos...@marvell.com>; Neta Zur 
Hershkovits <n...@marvell.com>; Kinney, Michael D <michael.d.kin...@intel.com>
Subject: Re: [edk2] [PATCH 1/1] EmbeddedPkg: Implement NorFlashLib

Hi Leif,

2017-10-31 8:58 GMT+01:00 Leif Lindholm <leif.lindh...@linaro.org>:
> On Mon, Oct 30, 2017 at 09:30:25PM +0100, Marcin Wojtas wrote:
>> The SPI NOR flash drivers which base on ArmPlatformPkg's NorFlashDxe 
>> usually make use of static declarations of the flash instances with 
>> their type and parameters. As a result it implies hardcoding the 
>> exact way flash handling, not to mention the code does not look very 
>> nice. Much better solution would be obtaining the flash ID and hence 
>> its description in runtime.
>>
>> Because JEDEC compliant SPI NOR devices allow to obtain their ID with 
>> READ_ID command (0x9f), implement a NorFlashLib that gives access to 
>> the NOR flash data, such as name, page size, sector
>> (block) size and others, of more than 50 different models.
>> The new library user should pass an output array issuing READ_ID 
>> command to the GetNorFlashInfo () routine - if the match is found, an 
>> allocated (optionally for RT) pool with the flash description will be 
>> returned.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <m...@semihalf.com>
>> ---
>>  EmbeddedPkg/EmbeddedPkg.dec |   1 +
>>  EmbeddedPkg/Include/Library/NorFlashInfoLib.h   |  84 
>>  EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.c   | 225 
>> 
>>  EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf |  34 +++
>>  4 files changed, 344 insertions(+)
>>
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec 
>> b/EmbeddedPkg/EmbeddedPkg.dec index 52482af..aa551ab 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -45,6 +45,7 @@
>>EblNetworkLib|Include/Library/EblNetworkLib.h
>>GdbSerialLib|Include/Library/GdbSerialLib.h
>>DebugAgentTimerLib|Include/Library/DebugAgentTimerLib.h
>> +  NorFlashInfoLib|Include/Library/NorFlashInfoLib.h
>>
>>DtPlatformDtbLoaderLib|Include/Library/DtPlatformDtbLoaderLib.h
>>
>> diff --git a/EmbeddedPkg/Include/Library/NorFlashInfoLib.h 
>> b/EmbeddedPkg/Include/Library/NorFlashInfoLib.h
>> new file mode 100644
>> index 000..ae0e45f
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Library/NorFlashInfoLib.h
>> @@ -0,0 +1,84 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017 Marvell International Ltd.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of 
>> +the BSD License
>> +*  which accompanies this distribution.  The full text of the 
>> +license may be found at
>> +*  
>> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
>> +ensource.org%2Flicenses%2Fbsd-license.php=02%7C01%7Cpankaj.bans
>> +al%40nxp.com%7C753424b872b346c9c76f08d52037acfc%7C686ea1d3bc2b4c6fa9
>> +2cd99c5c301635%7C0%7C0%7C636450345874702676=cxqjBkGwxPBYUu%2Fh
>> +Rwn8xdMheRjl%2BXBokEA74oIyaTw%3D=0
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
>> +BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> IMPLIED.
>> +*
>> +**/
>> +
>> +#ifndef __NOR_FLASH_ID_LIB_H__
>> +#define __NOR_FLASH_ID_LIB_H__
>> +
>> +#include 
>> +
>> +#define NOR_FLASH_MAX_ID_LEN6
>> +
>> +typedef struct {
>> +  /* Device name */
>> +  UINT16 *Name;
>> +
>> +  /*
>> +   * JEDEC ID
>> +   */
>> +  UINT8  Id[NOR_FLASH_MAX_ID_LEN];
>> +  UINT8  IdLen;

Re: [edk2] Boot delay due to network devices

[Pankaj Bansal]

Hi Laszlo,

Thank you for your prompt reply.
But it's not like that we do not want to boot from network device ever.
We have eight network devices, out of which one would be used for booting the 
kernel.
We do not know which one of the eight devices, would be connected to network 
when system is in use.
Moreover network configuration can be changed, when the system is running. i.e. 
we can disconnect one device and connect other and try to boot from it.
Therefore we need to initialize all the network devices.

However the network device initialization (Snp->Initialize) expects the link 
status of network device to be updated after phy auto negotiation.
For devices which are not connected to network, this will result in phy auto 
negotiation timeout after 5 seconds.
I want to reduce this delay.
Can we attempt phy auto negotiation for only those network devices, which we 
will be using for booting (or for any network activity for that matter)
In other words for only those devices, for which we will be calling 
Snp->Transmit and Snp->Receive functions?

Thanks & Regards,
Pankaj Bansal

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Friday, October 06, 2017 3:18 PM
To: Pankaj Bansal <pankaj.ban...@nxp.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] Boot delay due to network devices

On 10/06/17 11:23, Pankaj Bansal wrote:
> Hello edk2 team,
> 
> We are getting boot time delay in edk2 in our armv8 based platform due to 
> network devices.
> We have implemented the network device drivers in our platform as SNP device 
> drivers.
> We have total of eight network interfaces (eight SNP protocols).
> Out of eight only one or two network interfaces are connected to network at a 
> time; rest are disconnected.
> When we boot the system then all the devices are detected (Snp->Start) and 
> initialized (Snp->Initialize).
> During Snp->Initialize(), the phy auto negotiation is started and driver 
> waits for auto negotiation to complete for maximum of 5 seconds.
> For each network device that is not connected, the system spends 5 seconds 
> before exiting out of Initialize.
> 
> We don't want to use these network devices for boot, still the time is being 
> spent to check their status.
> Is there some way we can skip this delay due to phy auto negotiation during 
> boot ?
> If I move phy auto negotiation to Snp->Transmit and Snp->Receive, will this 
> violate the SNP protocol guidelines ?

Do not connect the devices that you don't intend to boot off of -- avoid 
calling gBS->ConnectController() on them. (Equivalently, don't call
EfiBootManagerConnectXxx() functions from  UefiBootManagerLib that result in 
such gBS->ConnectController() calls internally.)

What devices are connected at boot is platform policy, implemented in the 
platform's PlatformBootManagerLib instance. In your PlatformBootManagerLib 
instance, you probably call
EfiBootManagerConnectAll() somewhere.

Replace it with various, more specific, EfiBootManagerConnectXxx() calls, as 
appropriate, from
"MdeModulePkg/Include/Library/UefiBootManagerLib.h":

- EfiBootManagerConnectDevicePath()
- EfiBootManagerConnectAllDefaultConsoles()
- EfiBootManagerConnectConsoleVariable()
- EfiBootManagerConnectVideoController()

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] Boot delay due to network devices

[Pankaj Bansal]

Hello edk2 team,

We are getting boot time delay in edk2 in our armv8 based platform due to 
network devices.
We have implemented the network device drivers in our platform as SNP device 
drivers.
We have total of eight network interfaces (eight SNP protocols).
Out of eight only one or two network interfaces are connected to network at a 
time; rest are disconnected.
When we boot the system then all the devices are detected (Snp->Start) and 
initialized (Snp->Initialize).
During Snp->Initialize(), the phy auto negotiation is started and driver waits 
for auto negotiation to complete for maximum of 5 seconds.
For each network device that is not connected, the system spends 5 seconds 
before exiting out of Initialize.

We don't want to use these network devices for boot, still the time is being 
spent to check their status.
Is there some way we can skip this delay due to phy auto negotiation during 
boot ?
If I move phy auto negotiation to Snp->Transmit and Snp->Receive, will this 
violate the SNP protocol guidelines ?

Thanks & Regards,
Pankaj Bansal



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

Re: [edk2] Storing Non volatile variables on SD/NAND

[Pankaj Bansal]

This use case also arises for single-board systems like raspberry-pi, which do 
not have an onboard flash.
The boot firmware/bootloader as well as operating system are loaded from SD 
card.
https://www.raspberrypi.org/documentation/configuration/config-txt/

Thanks & Regards,
Pankaj Bansal

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Andrew 
Fish
Sent: Wednesday, September 20, 2017 10:48 AM
To: Udit Kumar <udit.ku...@nxp.com>
Cc: edk2-devel@lists.01.org; Vladimir Olovyannikov 
<vladimir.olovyanni...@broadcom.com>; olivier.mar...@arm.com; Ard Biesheuvel 
<ard.biesheu...@linaro.org>
Subject: Re: [edk2] Storing Non volatile variables on SD/NAND


> On Sep 19, 2017, at 10:09 PM, Udit Kumar <udit.ku...@nxp.com> wrote:
> 
>>> On Sep 19, 2017, at 9:27 PM, Udit Kumar <udit.ku...@nxp.com> wrote:
>>> 
>>> 
>>>> On 18 September 2017 at 22:28, Udit Kumar <udit.ku...@nxp.com> wrote:
>>>>> Thanks Vladimir,
>>>>> With your design, you did delayed write to eMMC due to sharing 
>>>>> with OS.  But it works for you:) Say if eMMC controllers offers 
>>>>> you a status bit, if eMMC storage is being used for not. Then this 
>>>>> could be possible to
>>>> update at run time, both OS/UEFI needs to check and wait if 
>>>> controller is being used.
>>>> 
>>>> That is the problem right there. The nice thing about a firmware 
>>>> spec is that you don't have to care about how it was implemented if 
>>>> you adhere to
>> the API rules.
>>> 
>>> Yup, we are fine as long as long UEFI firmware is stored on dedicated media.
>>> 
>>>> Imposing additional restrictions (such as requiring the OS to be 
>>>> careful about not using the eMMC when it may be in use by the
>>>> firmware) defeats the purpose of using UEFI, since you won't be 
>>>> able to use a
>> generic OS anyway.
>>>> 
>>> 
>>> Hmm,  so far, I haven't come across where UEFI specs says, we need a 
>>> separate Storage for firmware. (May be I missed some part of specs) 
>>> Irrespective of storage media, we have this problem if OS and UEFI 
>>> shares same storage.
>>> 
>> 
>> Udit,
>> 
>> Can you point out the spec that states you can't boot Linux and 
>> Windows at the same time on a PC? :)
>> 
>> When you write a spec it is not practical do document what is not 
>> possible, you can only document the API the rest is implied by the 
>> implementation. So for example the UEFI spec does not document why 
>> the firmware and OS can't share a hardware device, just like you 
>> can't have 2 operating systems running on bare metal at the same 
>> time. It is a little like Occam's Razor the reason that the firmware 
>> and the OS can not share a hardware device is the mechanics of how to 
>> share a hardware device is not defined in the spec, thus it is not part of 
>> the API and not possible.
> 
> Right,  This is left on implementation how to put firmware and OS.
> Ideally, keeping both storage separate  is best case, no need to sync between 
> two.
> 
> My reply to Ard, was to highlight that in any case (NOR or eMMC /NAND) 
> if we are keeping OS and firmware on same storage, we will have same 
> issue not limited to  eMMC.
> 
> For some requirement, if we need to keep firmware and OS on same 
> media, Then implementation should make sure there is exclusive access 
> (be it NOR controller, SD controller etc).
> 

Udit,

Sorry I'm a little swamped on my email right now and might be a little behind 
on the thread

Yea the only way to realistically Implement an EFI runtime service in UEFI is 
to have UEFI own the hardware device. There is no architecture for sharing the 
device, and the type of device is not really relevant. 

Thanks,

Andrew Fish

> Thanks
> Udit
> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>>>> For sure,  some synchronization issues need to be ironed out (or 
>>>>> maybe I am
>>>> just dreaming here).
>>>>> 
>>>>> On part 2) where you forked VariableRuntime driver , could we 
>>>>> think of updating VariableRuntime driver, to support non-XIP or 
>>>>> memory mapped
>>>> devices.
>>>>> 
>>>> 
>>>> I think being able to support non-memorymapped FV volumes for the 
>>>> variable store would be a big improvement. This does require 
>>>> changes to both the FaultTolerantWrite

Re: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Pankaj Bansal]

Hi Zeng,

Thanks for suggestion.
I agree, that this looks nicer and gives more details.
Please push the patch with comment updates and signatures as you have mentioned.

Thanks & Regards,
Pankaj Bansal

-Original Message-
From: Zeng, Star [mailto:star.z...@intel.com] 
Sent: Tuesday, September 19, 2017 8:30 AM
To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>; Laszlo Ersek (ler...@redhat.com) 
<ler...@redhat.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change 
serial attributes

Pankaj,

Thanks for your explanation at 
https://lists.01.org/pipermail/edk2-devel/2017-September/014832.html about the 
execute flow to reproduce the issue.
How about also include the explanation to the commit log like below?

==
Issue : When try to change serial attributes using sermode command, the default 
values are set with the execute flow as below.

The sermode command calls SerialSetAttributes, which sets H/W attributes of 
Serial device. After that the SerialIo protocol is reinstalled, which causes 
MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and 
then start. This in turn calls SerialReset, which undoes changes of 
SerialSetAttributes.

Cause : The SerialReset command resets the attributes' values to default.
Fix : Serial Reset command should set the attributes which have been changed by 
user after calling SerialSetAttributes.
==

Another minor comment, how about update "COM port" to be "Serial Port" in the 
change?


If you agree with above comments, you do not need to send an update patch and I 
will help push the patch with the updates I comment above and Reviewed-by: Star 
Zeng <star.z...@intel.com>, and of course Regression-tested-by: Laszlo Ersek 
<ler...@redhat.com>.


Thanks,
Star
-Original Message-
From: Zeng, Star
Sent: Monday, September 18, 2017 6:12 PM
To: Pankaj Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change 
serial attributes

Pankaj,

Thanks for the update.

I raised a question in the V1 patch review, could you help kindly provide the 
feedback?
"how you reproduce the issue by sermode command as I see sermode command only 
calls SerialIo->SetAttributes() but not SerialIo->Reset()"


Thanks,
Star
-Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Pankaj 
Bansal
Sent: Monday, September 18, 2017 3:43 PM
To: edk2-devel@lists.01.org
Cc: Pankaj Bansal <pankaj.ban...@nxp.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change 
serial attributes

Issue : When try to change serial attributes using sermode command, the default 
values are set Cause : The SerialReset command resets the attributes' values to 
default Fix : Serial Reset command should set the attributes which have been 
changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 -
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c 
b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUSStatus;
-  EFI_TPL   Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout   = 1000 * 1000;
-  This->Mode->BaudRate  = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits  = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity= (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits  = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-  mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-  mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-  mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
- ) {
-gBS->RestoreTPL (Tpl);
-return EFI_SUCCESS;
-  }
-
-  //
-  // Updat

Re: [edk2] [PATCH] Fix not able to change serial attributes

[Pankaj Bansal]

Hi Laszlo, Zeng

Thanks for patch review.

@Zeng: The sermode command calls SerialSetAttributes, which sets H/W attributes 
of Serial device.
After that the SerialIo protocol is reinstalled, which causes 
MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and 
then start.
This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

Thanks & Regards,
Pankaj Bansal
-Original Message-
From: Zeng, Star [mailto:star.z...@intel.com] 
Sent: Monday, September 18, 2017 3:52 PM
To: Laszlo Ersek <ler...@redhat.com>; Heyi Guo <heyi@linaro.org>; Pankaj 
Bansal <pankaj.ban...@nxp.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH] Fix not able to change serial attributes

Thanks for your good comments. :)
Since there is no clear description for the behavior of Reset() :(, I prone to 
align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and 
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the 
fix.


Laszlo and Gary, if you can help do some simple regression test with the patch, 
that will be better.


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Sunday, September 17, 2017 4:18 AM
To: Zeng, Star <star.z...@intel.com>; Pankaj Bansal <pankaj.ban...@nxp.com>; 
edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>
Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes

On 09/16/17 03:21, Zeng, Star wrote:
> Pankaj,
> 
> Thanks for the contribution.
> Could you help update the title to be like MdeModulePkg SerialDxe: XX?
> 
> In fact, I agree the fix as it matches the code at 
> MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and 
> IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c.
> But I am curious about how you reproduce the issue by sermode command as I 
> see sermode command only calls SerialIo->SetAttributes() but not 
> SerialIo->Reset().
> 
> 
> Ray, Laszlo,
> Do you have any comment?

Thanks for the CC.

The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It only 
says, "The Reset() function resets the hardware of a serial device." It doesn't 
define what state the device should return to after resetting the hardware.

Under the generic description of EFI_SERIAL_IO_PROTOCOL, we have

"The default attributes for all UART-style serial device interfaces are:
115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond timeout per 
character, no parity, 8 data bits, and 1 stop bit."

Now, the PCDs that are currently used in the code may differ from the above 
standard-mandated values, but that's the platform's responsibility. The point 
is that the UEFI spec seems to require that the device be returned to a 
predefined state, and the PCDs make that possible. I don't think that the 
argument in the commit message, "Serial Reset command should set the attributes 
which have been changed by user after calling SerialSetAttributes.", is 
consistent with the UEFI spec's intent.

However, I could easily be wrong about this, given especially that the other 
two SerialIo implementations already follow the suggested practice.

I guess I'll have to stay neutral on this patch. Hopefully it won't regress 
anything.

Thanks,
Laszlo


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Pankaj Bansal
> Sent: Friday, September 15, 2017 9:14 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.ban...@nxp.com>
> Subject: [edk2] [PATCH] Fix not able to change serial attributes
> 
> Issue : When try to change serial attributes using sermode command, the 
> default values are set Cause : The SerialReset command resets the attributes' 
> values to default Fix : Serial Reset command should set the attributes which 
> have been changed by user after calling SerialSetAttributes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66
> -
>  1 file changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index 43d33db..dc7e13a 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -220,7 +220,6 @@ SerialReset (
>)
>  {
>EFI_STATUSStatus;
> -  EFI_TPL   Tpl;
>  
>Status = SerialPortInitialize ();
>if (EFI_ERROR (Status)) {
> @@ -228,49 +227,17 @@ SerialReset (
>}
&

[edk2] [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes

[Pankaj Bansal]

Issue : When try to change serial attributes using sermode
command, the default values are set
Cause : The SerialReset command resets the attributes' values
to default
Fix : Serial Reset command should set the attributes which have
been changed by user after calling SerialSetAttributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
---
Changes in v2:
   - Modified Patch description

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 -
 1 file changed, 18 insertions(+), 48 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c 
b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 43d33db..dc7e13a 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -220,7 +220,6 @@ SerialReset (
   )
 {
   EFI_STATUSStatus;
-  EFI_TPL   Tpl;
 
   Status = SerialPortInitialize ();
   if (EFI_ERROR (Status)) {
@@ -228,49 +227,17 @@ SerialReset (
   }
 
   //
-  // Set the Serial I/O mode and update the device path
-  //
-
-  Tpl = gBS->RaiseTPL (TPL_NOTIFY);
-
-  //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
-  This->Mode->Timeout   = 1000 * 1000;
-  This->Mode->BaudRate  = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits  = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity= (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits  = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
-  // Check if the device path has actually changed
-  //
-  if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
-  mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits &&
-  mSerialDevicePath.Uart.Parity   == (UINT8) This->Mode->Parity &&
-  mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits
- ) {
-gBS->RestoreTPL (Tpl);
-return EFI_SUCCESS;
-  }
-
-  //
-  // Update the device path
+  // Go set the current attributes
   //
-  mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate;
-  mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits;
-  mSerialDevicePath.Uart.Parity   = (UINT8) This->Mode->Parity;
-  mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits;
-
-  Status = gBS->ReinstallProtocolInterface (
-  mSerialHandle,
-  ,
-  ,
-  
-  );
-
-  gBS->RestoreTPL (Tpl);
+  Status = This->SetAttributes (
+   This,
+   This->Mode->BaudRate,
+   This->Mode->ReceiveFifoDepth,
+   This->Mode->Timeout,
+   (EFI_PARITY_TYPE) This->Mode->Parity,
+   (UINT8) This->Mode->DataBits,
+   (EFI_STOP_BITS_TYPE) This->Mode->StopBits
+   );
 
   return Status;
 }
@@ -513,11 +480,6 @@ SerialDxeInitialize (
 {
   EFI_STATUSStatus;
 
-  Status = SerialPortInitialize ();
-  if (EFI_ERROR (Status)) {
-return Status;
-  }
-
   mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
   mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
   mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
@@ -529,6 +491,14 @@ SerialDxeInitialize (
   mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
 
   //
+  // Issue a reset to initialize the COM port
+  //
+  Status = mSerialIoTemplate.Reset ();
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  //
   // Make a new handle with Serial IO protocol and its device path on it.
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
-- 
2.7.4

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

[edk2] (no subject)

[Pankaj Bansal]

Hi Laszlo, Zeng

Thanks for patch review.

@Zeng: The sermode command calls SerialSetAttributes, which sets H/W attributes 
of Serial device.
After that the SerialIo protocol is reinstalled, which causes 
MdeModulePkg/Universal/Console/TerminalDxe
and MdeModulePkg/Universal/Console/ConPlatformDxe drivers' bindings to stop and 
then start.
This in turn calls SerialReset, which undoes changes of SerialSetAttributes.

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

[edk2] UART Baud rate change code flow

[Pankaj Bansal]

Hello Edk2 team,

We are trying to change the UART Baud rate dynamically from UEFI shell using 
"sermode" command in our ARM64 based platform.
I see the code flow of sermode call as below :

ShellCommandRunSerMode() -> SerialSetAttributes() ->
SerialPortSetAttributes()

SerialSetAttributes() <-
TerminalDriverBindingStart() -> 
SerialSetAttributes()  ->   SerialPortSetAttributes() 
TerminalDriverBindingStart() <- 
SerialSetAttributes()  <- 
TerminalConInReset() -> 
SerialReset () ->SerialPortInitialize()
TerminalConInReset() <- 
SerialReset() <- 
TerminalConInTimerHandler() ->  
SerialSetAttributes() ->SerialPortSetAttributes() 
TerminalConInTimerHandler() <-  
SerialSetAttributes()
ShellCommandRunSerMode() <-

I am not able to understand why SerialSetAttributes and SerialReset are being 
called, while the first call to SerialSetAttributes has not yet completed.
Is this code flow right ? am I missing something ?

Thanks & Regards,
Pankaj Bansal

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

[edk2] AARCH64 : C Model Small Alignment requirements

[Pankaj Bansal]

Hello Edk2 team,

We are facing a compilation error in our ARMV8 based NXP board.
We are using c model small with GCC49 and GCC5.

"WriteSections64(): %s  due to its size (> 1 MB), this module requires 4 KB 
section alignment."

With below GCC options, this error is not observed

[BuildOptions]
   GCC:*_*_AARCH64_CC_FLAGS = -mcmodel=small
+  GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x1000

My question is, is this necessary to use page size 4K with c model small ?

P.S : This compilation error is not observed with default GCC49/GCC5 options 
(i.e. C Model tiny and Page Size 0x20)

Thanks & Regards,
Pankaj Bansal

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

[edk2] ArmPkg/PlatformBootManagerLib : Multiple UEFI Shell Boot entries

[Pankaj Bansal]

Hello Edk2 team,

We are observing that sometimes "UEFI Shell" entry is created even though it is 
present in boot manager menu in our ARMV8 NXP board
When this happens multiple times, we see multiple entries of "UEFI Shell" in 
boot manager menu:

/--\
|Boot Manager  |
\--/

 Device Path :
   Boot Manager Menu MemoryMapped(0xB,0xFF5
 B1000,0xFFAC17D7)/FvFi
   UEFI Shellle(7C04A583-9E3E-4F1C-
   UEFI Misc Device  AD65-E05268D0B4D1)
   UEFI Misc Device 2
   UEFI Misc Device 3
   UEFI Misc Device 4
   UEFI Shell
   UEFI PXEv4 (MAC:6805CA04D56A)
   UEFI Shell
   UEFI Shell
   UEFI Shell
   UEFI Shell
   UEFI Shell
   v 
/--\
|  |
| ^v=Move Highlight   =Select Entry  Esc=Exit   |
\--/

We see that commit 
https://github.com/tianocore/edk2/commit/0e2c6c552990edcd6352c2395860cb0df62b158d
 can fix this problem.

Remove any boot options that point to binaries built into the firmware 
and
have become stale due to any of the following:
- FvMain's base address or size changed (historical -- see commit
  
https://github.com/tianocore/edk2/commit/e191a3114f4c8fc0a05e4dc7bb72935f18ff4de9),
- FvMain's FvNameGuid changed,
- the FILE_GUID of the pointed-to binary changed,
- the referenced binary is no longer built into the firmware.

For example, multiple such "EFI Internal Shell" boot options can 
coexist.
They technically differ from each other, but may not describe any 
built-in
shell binary exactly. Such options can accumulate in a varstore over 
time,
and while they remain generally bootable (thanks to the efforts of
BmGetFileBufferByFvFilePath()), they look bad.

Filter out any stale options.


But this commit is for ArmVirtPkg/PlatformBootManagerLib, while we use 
ArmPkg/PlatformBootManagerLib.
Can this commit be ported from ArmVirtPkg to ArmPkg? Are there any side 
effects/ precautions for doing so?

Thanks & Regards,
Pankaj Bansal

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