Re: [edk2] patch for addition of new samsung exynos 4210 Soc support

Subject: Re: [edk2] patch for addition of new samsung exynos 4210 Soc support

From: Andrew Fish <afish@apple.com>

To: edk2-devel@lists.sourceforge.net

Date: 2011-06-10 15:33:14

Girish,

If you need platform specific features for GPIO I think there are two general design approaches you can take:
1) Make a platform specific GPIO protocol. This means you add a .h file, change the name of the interface (maybe add something about the SOC), an generate a new GUID. 
2) Use the existing GPIO protocol as is, an put a platform specific extension protocol on the same handle that has the extra features that are specific to your SOC. 

In both cases the extra features are scoped to your project. Since interfaces are named by GUID, you should never have a conflict with any other EFI binary. For shared C code the only possible conflict would be the structure and global name for the GUID. 

Hope this helps. 

Thanks,

Andrew Fish





On Jun 9, 2011, at 9:13 PM, Girish KS wrote:

Hello Andrew,
In my case i am using the GPIO pins for LCD during the DXE phase. My default console is the Graphics Console. I have to set the driving strength of the LCD module pins When the constructor of the graphics driver is called. I cannot do that as u mentioned in the early boot stage.
I cannot fix the driving strength of the pins in the early stage, as they can be used by other IP, and it may require some other value of driving value.

If the mentioned  pointer is provided, then during the DXE phase the pins can be muxed between IP's flexibly. eg: I mean the same pin of SD card detect(when card reader is not being used), can be used for other purpose with its own driving strength requirement.

please check my SamsungPlatformPkg/ExynosPkg/GraphicsConsoleDxe/ExynosGop.c to understand the requirement.

if you have any alternate suggestion than what is done pls let me know. If you feel the necessity for the Api pls update it in the protocol

thanks and regards
Girish K S


On Wed, Jun 8, 2011 at 6:45 PM, Andrew Fish <afish@apple.com> wrote:
Olivier,

Thanks for the detailed review.

If I look at the GPIO abstraction from 10,000 feet and abstract away hardware implementation I think you quickly come to this conclusion that this interface should be as simple and generic as possible.  The original protocol was our fault and we just grabbed it from another project. 

I would posit the normal boot flow for things like pin muxing, driver strength, pull up/down, are probably set in a table early in boot as part of the platform boot. A generic GPIO abstraction can probably get away with reading the pin and setting the pins value in a generic way. I think in our case it was diagnostics that wanted to mess around with pins that had not been set by firmware and that is why the Embedded GPIO protocol ended up getting some extra, non generic stuff. 

Given this I would not advocate adding more functions to this protocol. If anything it may make  more sense to make it simpler? I think the target of the Embedded GPIO protocol should be abstracting generic GPIO functions from a driver, in combination with a PCD that defines which GPIO the driver should use on a given platform. So an example wold be the SDXC driver (just making this example up) using a GPIO pin to tell if a card is inserted. This SDXC driver does not need to know the complexities of the board layout and routing, it just needs abstracted access to a given GPIO pin on the system and be able to read a 0 or 1.

Hope this helps.

Andrew Fish





On Jun 8, 2011, at 3:08 PM, Olivier Martin wrote:

I have just finished to review Girishs patches (see modified patches in the attached files).
 
# 0001-Removed-a-statement-at-the-end-of-file-to-make-compi.patch
 
- I did not keep this patch. I fixed the ARMGCC build issue last week.
- the dummy padding in the Exception.S files was a workaround for the required alignment of the Vector Tables.
This workaround has been replaced by setting a 32bit alignment for the Sec modules in the FDF file.
See patch 0003-SamsungPlatformPkg-SmdkBoardPkg-Force-the-Sec-module.patch
 
 
# 0002-GPIO-set-driving-strength-function-pointer-added.patch
 
- I am not familiar enough with the GPIO interface to commit this patch.
Your patch also breaks the BeagleBoard package. You have not defined EMBEDDED_GPIO_DRV for the Omap35xxPkg/Gpio.
 
- It seems to me your proposal is quite tied to your GPIO controller.
From the OMAP35x TRM, there is a field GATINGRATIO into the GPIO_CTRL register that has the following ratio:
0x0: Functional clock is interface clock.
0x1: Functional clock is interface clock divided by 2.
0x2: Functional clock is interface clock divided by 4.
0x3: Functional clock is interface clock divided by 8.
 
It means these ratio cannot be mapped with your proposed interface.
 
- Another comment, the name of the method is not clear enough. Why 'SetDrv' and not 'SetStrength' for setting the Strength ?
 
Additional comments or changes are welcome.
 
 
# 0003-Added-the-Exynos-4210-driver-package.patch and 0004-Smdk-package-based-on-exynos4210.patch
 
- I edited your patches to move the 'ExynosPkg' and 'SmdkBoardPkg' from 'ArmPlatformPkg' to 'SamsungPlatformPkg'.
- I would advise you to rename your SmdkBoardPkg.dsc and SmdkBoardPkg.fdf into SmdkBoardPkg-Exynos.dsc and SmdkBoardPkg-Exynos.fdf. If in the future you use your SmdkBoardPkg for other SoC (such as ArmPlatformPkg/ArmVExpressPkg).
See patch 0004-SamsungPlatformPkg-SmdkBoardPkg-Renamed-SmdkBoardLib.patch
 
 
# 0004-SamsungPlatformPkg-SmdkBoardPkg-Renamed-SmdkBoardLib.patch
 
-In addition to rename SmdkBoardPkg-Exynos.(dsc|fdf), I also renamed SmdkBoardLibRTSM into SmdkBoardLib as RTSM stands for 'Real-Time System Model'.
 
 
# 0005-SamsungPlatformPkg-SmdkBoardPkg-Fix-build.patch
 
- In SmdkBoardPkg.dec, the 'Include' folder does not exist
- The PL310L2Cache's initialization function has been recently modified to take the L2 latencies. I fixed the build by setting the longest latencies for your controller.
- I had to comment out the ACPI modules to build SmdkBoardPkg
 
 
# As I asked you offline, it would be nice to provide some documentation to set up UEFI on your Exynos 4210 SmdkBoard.
 
 
Regards,
Olivier
 
From: Andrew Fish [mailto:afish@apple.com] 
Sent: 08 June 2011 17:29
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] patch for addition of new samsung exynos 4210 Soc support
 
Girish,
 
Also the ArmPlatformPkg is for ARM Ltd, reference platforms, an not all ARM CPU based projects. 
 
Notice the Omap35xxPkg and BeagleBoardPkg that were checked in as an example ARM platform port. 
 
Andrew Fish
 
 


 
On Jun 8, 2011, at 9:17 AM, Andrew Fish wrote:


Girish,
 
Have you signed the contributors agreement? 
 
It is also good to break up the patches and name the package the patch is targeting and also address that mail to the maintainer .
 
so like:
[Patch] ArmPlatformPackage fix gcc compile issues
 
Olivier, 
 
....
 
 
Also in this patch why do you do:
+Dummy1: .word 0
+Dummy2: .word 0
 
vs
.align 3
 
Different folks own different packages, so thats why you need to target the patches at the maintainers. 
 
Andrew Fish
 
 
 
 
On Jun 8, 2011, at 1:21 AM, Girish KS wrote:


Hello Maintainers,


This is the re-release of my previos patch. I have changed the fdf file name

and the commit sign off name is also changed which created confusion.

Sorry for the same. please consider this patch.

   PS: May be the mail was not delivered in my previous attempt

regards
Girish K S
 

<0001-Removed-a-statement-at-the-end-of-file-to-make-compi.patch><0002-GPIO-set-driving-strength-function-pointer-added.patch><0003-Added-the-Exynos-4210-driver-package.patch><0004-Smdk-package-based-on-exynos4210.patch><0005-Changed-the-fdf-file-name.patch>------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
<0005-SamsungPlatformPkg-SmdkBoardPkg-Fix-build.patch><0001-Added-the-Exynos-4210-driver-package.patch.patch><0002-Smdk-package-based-on-exynos4210.patch.patch><0003-SamsungPlatformPkg-SmdkBoardPkg-Force-the-Sec-module.patch><0004-SamsungPlatformPkg-SmdkBoardPkg-Renamed-SmdkBoardLib.patch>------------------------------------------------------------------------------

EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel


------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel


------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel