Hi Leif,

Thanks. I'd be happy to do it after November, once the pressure gets smaller. Can you please point me the exact soltion example from Hisilicon?

As for the status, still to go:
- verify sata with new silicon (the board is still on its way to our office) - it is supposed to be quirkless, I keep fingers crossed it will work with generic driver
- submit sd/mmc driver- Jan is preparing patchset at the moment
- try fupdate with ShellDynamic protocol

And that's all. We'll be in touch.

Thanks,
Marcin
Od: Leif Lindholm
Wysłano: ‎2016-‎11-‎16 10:00
Do: Marcin Wojtas
DW: Linaro UEFI Mailman List; Ard Biesheuvel; Neta Zur Hershkovits; Yehuda Yitschak; Haim Boot; Nitzan Zorea; semihalf-dabros-jan; Rafal Jaworowski
Temat: Re: [PATCH v3 0/6] Armada 7040 PciEmulation, SATA, USB

I think this is where it would have been useful to separate platform
and SoC (which really only Hisilicon have done so far). And that's not
a criticism of your port (hey, I've been reviewing it all the way,
we're all learning here).

At which point it's probably better to merge something clunky, rather
than trying to improve it incrementally.

So, long-term, what I think it would make sense to move towards would
be:
- An SoC library(/driver) (Armada70x0)
  - Defining all of the possible components and possible base
    addresses and dev count and coherency information.
  - A .dec containing per-port boolean Pcds.
- A platform port that only sets the Pcds to enable the components
  available to it.

But for this pass, let's just go with what we have:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

(But we're waiting for Ard's next spin, with a version to go into
OpenPlatformPkg, and you verifying that it works as expected.)

/
    Leif

On Tue, Nov 15, 2016 at 11:04:32PM +0100, Marcin Wojtas wrote:
> Hi Leif,
>
> How about this for now:
> 1. In PciEmulation:
> - Static struct comprising all possible Armada 70x0 XHCI/AHCI/SDHCI
> base addresses + dev count of each + information if they are
> dma-coherent.
> - Iterate for each device type separately
>
> 2. In dsc - for each device type separate PCD, e.g. for USB:
> gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x0, 0x1 }
> It means that port0 is disabled and port1 - enabled.
>
> Imo it would be flexible for various boards, also easy for next
> possible SoC supported in OPP - Armada 80x0 can fit into it, as it's a
> superset of A70x0. Actually I think in the structure there could be
> even more SoC's described this way.
>
> Please let me know your opinion.
>
> Best regards,
> Marcin
>
>
>
> 2016-11-15 22:36 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > So, I have no strong objection to this set, but I do have a
> > question/comment:
> >
> > The semicolon-separated-arrays-in-Pcds mechanism is now by far the
> > clunkiest aspect of this set.
> >
> > I can well see a near future in which we add a core library to consume
> > a static struct from a platform library and do the registration on.
> > At that point, I would like this code to move over to that.
> >
> > So is there any way we could delete all the Pcd juggling, stick a
> > small static struct and a much simpler iterator into Armada70x0Lib
> > from day one instead?
> >
> > Regards,
> >
> > Leif
> >
> > On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
> >> Hi,
> >>
> >> I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver:
> >> [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
> >>
> >> Code is also available in the github:
> >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-r20161115-2-upstream
> >>
> >> According to review comments, PCD usage is improved, as well as redundant
> >> includes or packages were removed.
> >>
> >> We still do not have boards with new SATA IP, but if this version of
> >> PciEmulation is accepted, let's keep it in hand until
> >> NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the
> >> end of November.
> >>
> >> I'm looking forward to your review.
> >>
> >> Best regards,
> >> Marcin
> >>
> >> Changelog
> >> v2 -> v3
> >>
> >> * Use static globals for PCD-related variables
> >> * Check with 'if' in runtime if they are correct
> >> * Return error on each kind of failure
> >> * Don't use arrays of DmaTypes/DevTypes for registering
> >>   NonDiscoverableDevices
> >> * Remove redundant protocols, includes, libraries
> >> * Add reviewed-by in patches 2-6
> >>
> >> v1 -> v2
> >>
> >> * Move to NonDiscoverablePciDeviceDxe
> >>
> >>
> >> Jan Dąbroś (4):
> >>   Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform
> >>   Platforms/Marvell: Enable USB stack for Armada70x0 platform
> >>   Platforms/Marvell: Enable two xHCI ports for Armada70x0 board
> >>   Platforms/Marvell: Enable SATA stack for Armada70x0 platform
> >>
> >> Marcin Wojtas (2):
> >>   Platforms/Marvell: Add PciEmulation driver
> >>   Platforms/Marvell: Enable SATA port for Armada70x0 board
> >>
> >>  .../Marvell/PortingGuide/PciEmulation.txt          |  46 ++++++++
> >>  Platforms/Marvell/Armada/Armada.dsc.inc            |  19 ++++
> >>  Platforms/Marvell/Armada/Armada70x0.dsc            |  10 ++
> >>  Platforms/Marvell/Armada/Armada70x0.fdf            |  18 ++++
> >>  Platforms/Marvell/Marvell.dec                      |   6 ++
> >>  Platforms/Marvell/PciEmulation/PciEmulation.c      | 119 +++++++++++++++++++++
> >>  Platforms/Marvell/PciEmulation/PciEmulation.inf    |  46 ++++++++
> >>  7 files changed, 264 insertions(+)
> >>  create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt
> >>  create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c
> >>  create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
> >>
> >> --
> >> 1.8.3.1
> >>