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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Sep 01, 2017 at 05:08:39PM +0200, Marcin Wojtas wrote:
> 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?

Yeah, exactly. (Although depending on point if view, maybe
Marvell/Armada80x0McBin should be SolidRun/MacchiatoBin.)
I'm hoping there could be a lot of sharing of common bits of files in
generic .dsc.inc/.fdf.inc - both here and under Silicon/Marvell.

> 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?

Well, if I was going to take a hard line on this, that was an option I
had during the initial import to OpenPlatformPkg. Since I didn't, this
is very much a "I would be happy to see this happen" kind of thing.

So it's sort of up to you.

But I'll throw in that I am planning to go into hiding for a couple of
days next week to try and once and for all create some generic config
fragments that can be reused across all platforms instead of copying
the same bits along everywhere. So it would make sense to wait with
any larger interventions until that work is complete.

/
    Leif

> >> 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