[if gte mso 9]>

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: Olivier Martin <Olivier.Martin@arm.com>

To: "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2011-06-09 07:08:56

I have just finished to review Girish’s 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.