On Wed, Nov 16, 2016 at 10:37:03AM +0100, marcin wojtas wrote:
Hi Leif,
Thanks. I'd be happy to do it after November, once the pressure gets smaller.
Great, thanks.
Can you please point me the exact soltion example from Hisilicon?
Well, the solution is simply that they split the SoC code apart from the platform code: https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/Chips/Hisilicon vs https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/Platforms/Hisilicon
You guys are still the trailblazers on the non-discoverable side :)
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
If not, it'd be useful to see the quirk as a standaloen patch, to see what could be do abot integrating it in one of the more reusable solutions.
- 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.
Sounds good - thanks!
/ Leif
Thanks, Marcin
-----Wiadomość oryginalna----- Od: "Leif Lindholm" leif.lindholm@linaro.org Wysłano: 2016-11-16 10:00 Do: "Marcin Wojtas" mw@semihalf.com DW: "Linaro UEFI Mailman List" linaro-uefi@lists.linaro.org; "Ard Biesheuvel" ard.biesheuvel@linaro.org; "Neta Zur Hershkovits" neta@marvell.com; "Yehuda Yitschak" yehuday@marvell.com; "Haim Boot" hayim@marvell.com; "Nitzan Zorea" nzorea@marvell.com; "semihalf-dabros-jan" jsd@semihalf.com; "Rafal Jaworowski" raj@semihalf.com 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:
- 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
- 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-...
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