Re: [platforms: PATCH 01/13] Marvell/Armada: Introduce platform initialization driver

[Marcin Wojtas <mw@semihalf.com>]

2017-10-10 17:03 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 10, 2017 at 04:45:10PM +0200, Marcin Wojtas wrote:
>> Hi Leif,
>>
>> 2017-10-10 16:37 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > On Mon, Oct 09, 2017 at 07:00:50PM +0200, Marcin Wojtas wrote:
>> >> In order to enable modification of dynamic PCD's for the libraries
>> >> and DXE drivers, this patch introduces new driver. It is
>> >> executed prior to other drivers. Mpp, ComPhy and Utmi libraries
>> >> initialization were moved from PrePi stage to DXE.
>> >>
>> >> To force the correct driver dispatch sequence, introduce a protocol GUID
>> >> and install the protocol as a NULL protocol when PlatInitDxe executes.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > What does Ard's Signed-off-by signify here?
>> > (I know the authorship on some of these is a bit blurred, since you've
>> > been working together, but I'd like to be clear.)
>>
>> These were the lines, introducing/installing protocol GUID stuff. It
>> was in a small separate patch, but I squashed it into bigger one.
>
> Personally, I would in this instance do:
> <me>
> Ard
> <me>
>
> It's verbose, but reasonably clear.
>

How about:

In order to enable modification of dynamic PCD's for the libraries
and DXE drivers, this patch introduces new driver. It is
executed prior to other drivers. Mpp, ComPhy and Utmi libraries
initialization were moved from PrePi stage to DXE.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>

To force the correct driver dispatch sequence, introduce a protocol GUID
and install the protocol as a NULL protocol when PlatInitDxe executes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>

?

Was that, what you meant?

>> >
>> >> ---
>> >>  Platform/Marvell/Armada/Armada.dsc.inc                        |  3 ++
>> >>  Platform/Marvell/Armada/Armada70x0.fdf                        |  5 +++
>> >>  Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.c     | 44 ++++++++++++++++++++
>> >>  Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.inf   | 44 ++++++++++++++++++++
>> >>  Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.c | 11 -----
>> >>  Platform/Marvell/Marvell.dec                                  |  5 +++
>> >>  6 files changed, 101 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>> >> index 89fb7e7..417bb0c 100644
>> >> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>> >> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>> >> @@ -378,6 +378,9 @@
>> >>    ArmPkg/Drivers/TimerDxe/TimerDxe.inf
>> >>    ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
>> >>
>> >> +  # Platform Initialization
>> >> +  Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.inf
>> >> +
>> >>    # Platform drivers
>> >>    Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> >>    MdeModulePkg/Bus/I2c/I2cDxe/I2cDxe.inf
>> >> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
>> >> index c861e78..763d76a 100644
>> >> --- a/Platform/Marvell/Armada/Armada70x0.fdf
>> >> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
>> >> @@ -89,6 +89,11 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>> >>
>> >>    INF MdeModulePkg/Core/Dxe/DxeMain.inf
>> >>
>> >> +  #
>> >> +  # Platform Initialization
>> >> +  #
>> >> +  INF Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.inf
>> >> +
>> >>    # PI DXE Drivers producing Architectural Protocols (EFI Services)
>> >>    INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>> >>    INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
>> >> diff --git a/Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.c b/Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.c
>> >> new file mode 100644
>> >> index 0000000..919454b
>> >> --- /dev/null
>> >> +++ b/Platform/Marvell/Armada/Drivers/PlatInitDxe/PlatInitDxe.c
>> >> @@ -0,0 +1,44 @@
>> >> +/** @file
>> >> +  Copyright (C) Marvell International Ltd. and its affiliates
>> >
>> > We normally need a year here as well.
>> > If Ard has co-authored parts, I guess we should have Linaro copyright
>> > notice on affected files as well.
>>
>> I have no problem with that, if you find it appropriate, given my
>> explanation above.
>
> Personally I dont mind much, but I think it would be more correct.
>

Ok, will add it in v2.