Re: [platforms: PATCH 01/11] Platform/Marvell/Documentation: Refactor PortingGuide

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

2017-09-01 16:36 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Sep 01, 2017 at 03:08:13PM +0200, Marcin Wojtas wrote:
>> From: Nir Erez <nerez@marvell.com>
>>
>> This patch introduces following improvements to the PortingGuide
>> * Replace split documentation with single file
>> * Align format to Doxygen constraints
>
> * Add Build description.
> (Comment on that below.)
>
> Since you are moving things around, I take this as license to
> bikeshed:
>
> I think we should start looking towards separating the development
> board and SoC portions, like is done for most other platforms. After
> all, the goal is to get additional platforms (not just the EVB).
>
> Not saying this is something that needs to happen overnight, but it
> would make sense to make a start by moving these docs to
> Silicon/Marvell/Documentation as part of this patch.

I can add it there, no problem. About Silicon vs Platform - I checked
the code and basically we'd have to move everything from
Platform/Marvell to Silicon/Marvell and leave just subdirectories with
board files (dsc, fdf + dtb):
Platform/Marvell/Armada70x0Db
Platform/Marvell/Armada80x0Db
Platform/Marvell/Armada80x0McBin
etc.
Is it what you mean?

I'm wondering when will be good moment for this - I'm rebasing 70 OPP
patches slowly in my extra time, so maybe after the SPI patchset? Or
better after we merge everything to Platform and then do the code
shifting? What is your feeling about it?

>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Documentation/Build.txt           |  58 ++++
>>  Platform/Marvell/Documentation/PortingGuide.txt    | 371 +++++++++++++++++++++
>>  .../Marvell/Documentation/PortingGuide/ComPhy.txt  |  45 ---
>>  .../Marvell/Documentation/PortingGuide/I2c.txt     |  20 --
>>  .../Marvell/Documentation/PortingGuide/Mdio.txt    |   7 -
>>  .../Marvell/Documentation/PortingGuide/Mpp.txt     |  48 ---
>>  .../Documentation/PortingGuide/PciEmulation.txt    |  31 --
>>  .../Marvell/Documentation/PortingGuide/Phy.txt     |  45 ---
>>  .../Marvell/Documentation/PortingGuide/Pp2.txt     |  35 --
>>  .../Marvell/Documentation/PortingGuide/Reset.txt   |   7 -
>>  .../Marvell/Documentation/PortingGuide/Spi.txt     |  16 -
>>  .../Documentation/PortingGuide/SpiFlash.txt        |  23 --
>>  .../Marvell/Documentation/PortingGuide/Utmi.txt    |  35 --
>
> Also, it would be nice if you could generate patches with
>   --stat=1000 --stat-graph-width=20
> as documented in Laszlo's unkempt guide:
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
> Makes the above summare _much_ easier to read.

No problem, will do.

>
>>  13 files changed, 429 insertions(+), 312 deletions(-)
>>  create mode 100644 Platform/Marvell/Documentation/Build.txt
>>  create mode 100644 Platform/Marvell/Documentation/PortingGuide.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/ComPhy.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/I2c.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Mdio.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Mpp.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/PciEmulation.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Phy.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Pp2.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Reset.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Spi.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/SpiFlash.txt
>>  delete mode 100644 Platform/Marvell/Documentation/PortingGuide/Utmi.txt
>>
>> diff --git a/Platform/Marvell/Documentation/Build.txt b/Platform/Marvell/Documentation/Build.txt
>> new file mode 100644
>> index 0000000..1162e2e
>> --- /dev/null
>> +++ b/Platform/Marvell/Documentation/Build.txt
>> @@ -0,0 +1,58 @@
>> +UEFI Build Instructions
>> +=======================
>> +
>> +For toolchain versions limitations please refer to edk2 wiki page:
>> +https://github.com/tianocore/tianocore.github.io/wiki/Using-EDK-II-with-Native-GCC
>> +
>> +Fully supported are gcc4.5 - gcc4.9, so possible {toolchain_name} are:
>> +     - GCC45
>> +     - GCC46
>> +     - GCC47
>> +     - GCC48
>> +     - GCC49
>> +     - GCC5
>> +
>> +Supported {platform} are:
>> +     - Armada70x0
>> +
>> +Supported {target} are
>> +     - DEBUG
>> +     - RELEASE
>> +
>> +Build procedure
>> +---------------
>> +1. Prerequisites:
>> +
>> +     Clone into edk2 repositories and apply Marvell patches (Please refer to
>> +     Release notes for instructions).
>> +
>> +2. Prepare environment:
>> +
>> +     2.1 Several packages will be needed to fully set up an edk2 build environment:
>> +
>> +                     # sudo apt-get install build-essential uuid-dev
>> +                     # sudo apt-get install lib32stdc++6 lib32z1
>> +
>> +     2.2 Set up EDK2 environment
>> +
>> +                     # source edksetup.sh
>> +
>> +     2.3 Build base tools
>> +
>> +                     # make -C BaseTools
>> +
>> +     2.4 Set {toolchain_name}_AARCH64_PREFIX to path to your cross compiler
>> +
>> +                     # export {toolchain_name}_AARCH64_PREFIX=/path/to/toolchain
>> +
>> +             Example:
>> +             --------
>> +                     # export GCC5_AARCH64_PREFIX=/opt/gcc-linaro-5.3.1-2016.05-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-
>> +
>> +3. Build EDK2 for selected {platform}:
>> +
>> +                     # build -a AARCH64 -t {toolchain_name} -b {target} -p OpenPlatformPkg/Platforms/Marvell/Armada/{platform}.dsc
>> +
>> +     Example for building edk2 for Armada70x0 platform with GCC5 for DEBUG:
>> +
>> +                     # build -a AARCH64 -t GCC5 -b DEBUG -p OpenPlatformPkg/Platforms/Marvell/Armada/Armada70x0.dsc
>
> The above all documents OpenPlatformPkg, so is now dated.
> My preference would be for this to become a Readme.md under
> Platform/Marvell, if you feel additional documentation to the
> top-level Readme.md to be necessary.

Right. How about only some minimal Readme.md as you suggest with the
list of available Marvell platforms and pointer to the main Readme
file?

>
> Regardless, it is unrelated to the rest of changes here, so would make
> sense as a separate patch.

Ok.

>
>> diff --git a/Platform/Marvell/Documentation/PortingGuide.txt b/Platform/Marvell/Documentation/PortingGuide.txt
>> new file mode 100644
>> index 0000000..8c3579e
>> --- /dev/null
>> +++ b/Platform/Marvell/Documentation/PortingGuide.txt
>> @@ -0,0 +1,371 @@
>> +UEFI Porting Guide
>> +==================
>> +
>> +This document provides instructions for adding support for new Marvell Armada
>> +board. For the sake of simplicity new Marvell board will be called "new_board".
>> +
>> +1. Create configuration files for new target
>> +     1.1 Create FDF file for new board
>> +
>> +      - Copy and rename PathToYourOpp/Platforms/Marvell/Armada/Armada70x0.fdf to
>> +        PathToYourOpp/Platforms/Marvell/Armada/new_board.fdf
>
> OpenPlatformPkg.

Sure, I will update all of them.

>
>> +      - Change the first no-comment line:
>> +        [FD.Armada70x0_EFI] to [FD.{new_board}_EFI]
>> +
>> +     1.2 Create DSC file for new board
>> +
>> +      - Add new_board.dsc file to PathToYourOpp/Platforms/Marvell/Armada directory
>
> OpenPlatformPkg.
>
>> +      - Insert following [Defines] section to new_board.dsc:
>> +
>> +                     [Defines]
>> +                       PLATFORM_NAME                  = {new_board}
>> +                       PLATFORM_GUID                  = {newly_generated_GUID}
>> +                       PLATFORM_VERSION               = 0.1
>> +                       DSC_SPECIFICATION              = 0x00010019
>> +                       OUTPUT_DIRECTORY               = {output_directory}
>> +                       SUPPORTED_ARCHITECTURES        = AARCH64
>> +                       BUILD_TARGETS                  = DEBUG|RELEASE
>> +                       SKUID_IDENTIFIER               = DEFAULT
>> +                       FLASH_DEFINITION               = {path_to_fdf_file}
>> +
>> +      - Add "!include Armada.dsc.inc" entry to new_board.dsc
>> +
>> +2. Driver support
>> + - According to content of files from PathToYourOpp/Documentation/Marvell/PortingGuide
>
> OpenPlatformPkg.
>
>> +   insert PCD entries into new_board.dsc for every needed interface (as listed below).
>> +
>> +3. Compilation
>> + - Refer to PathToYourOpp/Documentation/Marvell/Build.txt. Remember to change
>
> OpenPlatformPkg. (Also referencing the Build.txt.)
>
>> +   {platform} to new_board in order to point build system to newly created DSC file.
>> +
>> +4. Output file
>> + - Output files (and among others FD file, which may be used by ATF) are
>> +   generated under directory pointed by "OUTPUT_DIRECTORY" entry (see point 1.2).
>> +
>> +
>> +COMPHY configuration
>> +====================
>> +In order to configure ComPhy library, following PCDs are available:
>> +
>> +  - gMarvellTokenSpaceGuid.PcdComPhyDevices
>> +
>> +This array indicates, which ones of the ComPhy chips defined in
>> +MVHW_COMPHY_DESC template will be configured.
>> +
>> +Every ComPhy PCD has <Num> part where <Num> stands for chip ID (order is not
>> +important, but configuration will be set for first PcdComPhyChipCount chips).
>> +
>> +Every chip has 3 ComPhy PCDs and three of them comprise per-board lanes
>> +settings for this chip. Their format is unicode string, containing settings
>> +for up to 10 lanes. Setting for each one is separated with semicolon.
>> +These PCDs together describe outputs of PHY integrated in simple cihp.
>> +Below is example for the first chip (Chip0).
>> +
>> +  - gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes
>> +     (Unicode string indicating PHY types. Currently supported are:
>> +
>> +             { L"unconnected", L"PCIE0", L"PCIE1", L"PCIE2", L"PCIE3",
>> +               L"SATA0", L"SATA1", L"SATA2", L"SATA3", L"SGMII0",
>> +               L"SGMII1", L"SGMII2", L"SGMII3", L"QSGMII",
>> +               L"USB3_HOST0", L"USB3_HOST1", L"USB3_DEVICE",
>> +               L"XAUI0", L"XAUI1", L"XAUI2", L"XAUI3", L"RXAUI0",
>> +               L"RXAUI1", L"KR" } )
>> +
>> +  - gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds
>> +     (Indicates PHY speeds in MHz. Currently supported are:
>> +             { 1250, 1500, 2500, 3000, 3125, 5000, 6000, 6250, 1031 }  )
>> +
>> +  - gMarvellTokenSpaceGuid.PcdChip0ComPhyInvFlags
>> +     (Indicates lane polarity invert)
>> +
>> +Example
>> +-------
>> +
>> +               #ComPhy
>> +               gMarvellTokenSpaceGuid.PcdComPhyDevices|{ 0x1 }
>> +               gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|L"SGMII1;USB3_HOST0;SFI;SATA1;USB3_HOST1;PCIE2"
>> +               gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|L"1250;5000;10310;5000;5000;5000"
>> +
>> +
>> +PHY Driver configuration
>> +========================
>> +MvPhyDxe provides basic initialization and status routines for Marvell PHYs.
>> +Currently only 1518 series PHYs are supported. Following PCDs are required:
>> +
>> +  - gMarvellTokenSpaceGuid.PcdPhyConnectionTypes
>> +     (list of values corresponding to PHY_CONNECTION enum)
>> +  - gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg
>> +     (boolean - if true, driver waits for autonegotiation on startup)
>> +  - gMarvellTokenSpaceGuid.PcdPhyDeviceIds
>> +     (list of values corresponding to MV_PHY_DEVICE_ID enum)
>> +
>> +PHY_CONNECTION enum type is defined as follows:
>> +
>> +             typedef enum {
>> +                     0    PHY_CONNECTION_RGMII,
>> +                     1    PHY_CONNECTION_RGMII_ID,
>> +                     2    PHY_CONNECTION_RGMII_TXID,
>> +                     3    PHY_CONNECTION_RGMII_RXID,
>> +                     4    PHY_CONNECTION_SGMII,
>> +                     5    PHY_CONNECTION_RTBI,
>> +                     6    PHY_CONNECTION_XAUI,
>> +                     7    PHY_CONNECTION_RXAUI
>> +             } PHY_CONNECTION;
>> +
>> +MV_PHY_DEVICE_ID:
>> +
>> +             typedef enum {
>> +                     0    MV_PHY_DEVICE_1512,
>> +             } MV_PHY_DEVICE_ID;
>> +
>> +It should be extended when adding support for other PHY models.
>> +Thus in order to set RGMII for 1st PHY and SGMII for 2nd, PCD should be:
>> +
>> + gMarvellTokenSpaceGuid.PcdPhyConnectionTypes|{ 0x0, 0x4 }
>> +
>> +with disabled autonegotiation:
>> +
>> + gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
>> +
>> +assuming, that PHY models are 1512:
>> +
>> + gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
>> +
>> +
>> +MDIO configuration
>> +==================
>> +MDIO driver provides access to network PHYs' registers via EFI_MDIO_READ and
>> +EFI_MDIO_WRITE functions (EFI_MDIO_PROTOCOL). Following PCD is required:
>> +
>> +  - gMarvellTokenSpaceGuid.PcdMdioBaseAddress
>> +     (base address of SMI management register)
>> +
>> +
>> +I2C configuration
>> +=================
>> +In order to enable driver on a new platform, following steps need to be taken:
>> + - add following line to .dsc file:
>> +   OpenPlatformPkg/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> + - add following line to .fdf file:
>> +   INF OpenPlatformPkg/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> + - add PCDs with relevant values to .dsc file:
>> +     - gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57 }
>> +             (addresses of I2C slave devices on bus)
>> +     - gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0 }
>> +             (buses to which accoring slaves are attached)
>> +     - gMarvellTokenSpaceGuid.PcdI2cBusCount|2
>> +             (number of SoC's I2C buses)
>> +     - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
>> +             (base addresses of I2C controller buses)
>> +     - gMarvellTokenSpaceGuid.PcdI2cClockFrequency|200000000
>> +             (I2C host controller clock frequency)
>> +     - gMarvellTokenSpaceGuid.PcdI2cBaudRate|100000
>> +             (baud rate used in I2C transmission)
>> +
>> +
>> +PciEmulation configuration
>> +==========================
>> +Installation of various NonDiscoverable devices via PciEmulation driver is performed
>
> I think calling this PciEmulation is no longer accurate.

Our driver in the tree is called this way, what do you suggest instead?

Thanks,
Marcin