From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>

Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.

This uses the board revision to return a numeric value representing
the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).

Knowing the Pi family will help us set the SD card routing when we
introduce support for the Pi 4 and should also be easier to maintain
than if using individual model detection.

Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
RpiFirmwareGetModelName ().

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 64 ++++++++++++++++++++
 Platform/RaspberryPi/Include/Protocol/RpiFirmware.h          |  8 +++
 2 files changed, 72 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 9b4aa068857c..dd61ef089ca7 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -1,5 +1,6 @@
 /** @file
  *
+ *  Copyright (c) 2019, ARM Limited. All rights reserved.
  *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
  *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
  *
@@ -595,6 +596,8 @@ RpiFirmwareGetModelName (
     return "Raspberry Pi 3 Model B+";
   case 0x0E:
     return "Raspberry Pi 3 Model A+";
+  case 0x10:
+    return "Raspberry Pi Compute Module 3+";
   case 0x11:
     return "Raspberry Pi 4 Model B";
   default:
@@ -602,6 +605,66 @@ RpiFirmwareGetModelName (
   }
 }
 
+STATIC
+EFI_STATUS
+EFIAPI
+RPiFirmwareGetModelFamily (
+  OUT   UINT32 *ModelFamily
+  )
+{
+  EFI_STATUS                  Status;
+  UINT32                      Revision;
+  UINT32                      ModelId;
+
+  Status = RpiFirmwareGetModelRevision(&Revision);
+  if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Could not get the board revision: Status == %r\n",
+      __FUNCTION__, Status));
+    return EFI_DEVICE_ERROR;
+  } else {
+    ModelId = (Revision >> 4) & 0xFF;
+  }
+
+  switch (ModelId) {
+  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+  case 0x00:          // Raspberry Pi Model A
+  case 0x01:          // Raspberry Pi Model B
+  case 0x02:          // Raspberry Pi Model A+
+  case 0x03:          // Raspberry Pi Model B+
+  case 0x06:          // Raspberry Pi Compute Module 1
+  case 0x09:          // Raspberry Pi Zero
+  case 0x0C:          // Raspberry Pi Zero W
+      *ModelFamily = 1;
+      break;
+  case 0x04:          // Raspberry Pi 2 Model B
+      *ModelFamily = 2;
+      break;
+  case 0x08:          // Raspberry Pi 3 Model B
+  case 0x0A:          // Raspberry Pi Compute Module 3
+  case 0x0D:          // Raspberry Pi 3 Model B+
+  case 0x0E:          // Raspberry Pi 3 Model A+
+  case 0x10:          // Raspberry Pi Compute Module 3+
+      *ModelFamily = 3;
+      break;
+  case 0x11:          // Raspberry Pi 4 Model B
+      *ModelFamily = 4;
+      break;
+  default:
+      *ModelFamily = 0;
+      break;
+  }
+
+  if (*ModelFamily == 0) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Unknown Raspberry Pi model family : ModelId == 0x%x\n",
+      __FUNCTION__, ModelId));
+    return EFI_UNSUPPORTED;
+    }
+
+  return EFI_SUCCESS;
+}
+
 STATIC
 CHAR8*
 EFIAPI
@@ -1168,6 +1231,7 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
   RpiFirmwareGetModel,
   RpiFirmwareGetModelRevision,
   RpiFirmwareGetModelName,
+  RPiFirmwareGetModelFamily,
   RpiFirmwareGetFirmwareRevision,
   RpiFirmwareGetManufacturerName,
   RpiFirmwareGetCpuName,
diff --git a/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h b/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h
index e49d6e6132d9..e3287e3c040f 100644
--- a/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h
+++ b/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h
@@ -1,5 +1,6 @@
 /** @file
  *
+ *  Copyright (c) 2019, ARM Limited. All rights reserved.
  *  Copyright (c) 2016, Linaro Limited. All rights reserved.
  *
  *  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -102,6 +103,12 @@ CHAR8*
   INTN ModelId
   );
 
+typedef
+EFI_STATUS
+(EFIAPI *GET_MODEL_FAMILY) (
+  UINT32 *ModelFamily
+  );
+
 typedef
 EFI_STATUS
 (EFIAPI *GET_FIRMWARE_REVISION) (
@@ -143,6 +150,7 @@ typedef struct {
   GET_MODEL             GetModel;
   GET_MODEL_REVISION    GetModelRevision;
   GET_MODEL_NAME        GetModelName;
+  GET_MODEL_FAMILY      GetModelFamily;
   GET_FIRMWARE_REVISION GetFirmwareRevision;
   GET_MANUFACTURER_NAME GetManufacturerName;
   GET_CPU_NAME          GetCpuName;
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50690): https://edk2.groups.io/g/devel/message/50690
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 14/11/2019 16:07, Pete Batard wrote:
> +typedef
> +EFI_STATUS
> +(EFIAPI *GET_MODEL_FAMILY) (
> +  UINT32 *ModelFamily
> +  );
> +
>   typedef
>   EFI_STATUS
>   (EFIAPI *GET_FIRMWARE_REVISION) (
> @@ -143,6 +150,7 @@ typedef struct {
>     GET_MODEL             GetModel;
>     GET_MODEL_REVISION    GetModelRevision;
>     GET_MODEL_NAME        GetModelName;
> +  GET_MODEL_FAMILY      GetModelFamily;
>     GET_FIRMWARE_REVISION GetFirmwareRevision;
>     GET_MANUFACTURER_NAME GetManufacturerName;
>     GET_CPU_NAME          GetCpuName;

Is the RASPBERRY_PI_FIRMWARE_PROTOCOL structure expected to be 
externally consumed at any point?  If so, then adding a field in the 
middle of the structure without changing the associated GUID would break 
the ABI.

It's great to see the Pi 4 heading towards having UEFI support; I'm 
looking forward to trying it out as soon as it's ready.

Thanks,

Michael

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50700): https://edk2.groups.io/g/devel/message/50700
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Michael,

On 2019.11.14 16:36, Michael Brown wrote:
> On 14/11/2019 16:07, Pete Batard wrote:
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *GET_MODEL_FAMILY) (
>> +  UINT32 *ModelFamily
>> +  );
>> +
>>   typedef
>>   EFI_STATUS
>>   (EFIAPI *GET_FIRMWARE_REVISION) (
>> @@ -143,6 +150,7 @@ typedef struct {
>>     GET_MODEL             GetModel;
>>     GET_MODEL_REVISION    GetModelRevision;
>>     GET_MODEL_NAME        GetModelName;
>> +  GET_MODEL_FAMILY      GetModelFamily;
>>     GET_FIRMWARE_REVISION GetFirmwareRevision;
>>     GET_MANUFACTURER_NAME GetManufacturerName;
>>     GET_CPU_NAME          GetCpuName;
> 
> Is the RASPBERRY_PI_FIRMWARE_PROTOCOL structure expected to be 
> externally consumed at any point?

I don't really expect so. And even if that becomes the case, I think the 
platform is still new enough at this stage not to expect anyone to run 
afoul of ABI breakage.

> If so, then adding a field in the 
> middle of the structure without changing the associated GUID would break 
> the ABI.

I guess we'll take that into consideration if we modify this structure 
again, so thanks for pointing it out. But for now, I would say that this 
concern is mostly irrelevant.

> It's great to see the Pi 4 heading towards having UEFI support; I'm 
> looking forward to trying it out as soon as it's ready.

Well, if you are that eager to look into it, you can already play with 
the 'pi4_dev1' branches we have for edk2-platforms and edk2-non-osi at 
https://github.com/pftf which is what we are currently working with (and 
what this patch series is based on).

Right now, you should get video and you should also be able to run the 
UEFI Shell, but since we don't have xHCI, there's no USB support, so the 
only means of interacting with the firmware is through serial...

Oh, and you must have the official Device Tree on your SD card when 
booting if you want the serial baudrate to be properly set to the 
expected 115200 bauds. On RPI4, it appears that the new VideoCore does 
read the Device Tree behind the scenes to perform some of its own init...

If you build the firmware, a basic config.txt with the following should 
get you sorted:

arm_64bit=1
enable_uart=1
core_freq=250
enable_gic=1
armstub=RPI_EFI.fd
disable_commandline_tags=1

If you need more info, feel free to contact me off-list.

Regards,

/Pete

> 
> Thanks,
> 
> Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50701): https://edk2.groups.io/g/devel/message/50701
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
> 
> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
> 
> This uses the board revision to return a numeric value representing
> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
> 
> Knowing the Pi family will help us set the SD card routing when we
> introduce support for the Pi 4 and should also be easier to maintain
> than if using individual model detection.
> 
> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
> RpiFirmwareGetModelName ().

Can you drop the above line and include the below as 1/? in v2?

/
    Leif

From 59f01ff36ac7918e9ce166acbd3e963f638ab4b1 Mon Sep 17 00:00:00 2001
From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Date: Mon, 18 Nov 2019 17:47:06 +0000
Subject: [PATCH edk2-platforms 1/1] Platform/RPi: Add missing model name

add a missing entry for the "Raspberry Pi Compute Module 3+" in
RpiFirmwareGetModelName ().

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 9b4aa068857c..dcb434fabefe 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -1,5 +1,6 @@
 /** @file
  *
+ *  Copyright (c) 2019, ARM Limited. All rights reserved.
  *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
  *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
  *
@@ -595,6 +596,8 @@ RpiFirmwareGetModelName (
     return "Raspberry Pi 3 Model B+";
   case 0x0E:
     return "Raspberry Pi 3 Model A+";
+  case 0x10:
+    return "Raspberry Pi Compute Module 3+";
   case 0x11:
     return "Raspberry Pi 4 Model B";
   default:
-- 
2.20.1




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50846): https://edk2.groups.io/g/devel/message/50846
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2019.11.18 17:51, Leif Lindholm wrote:
> On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
>> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
>>
>> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
>>
>> This uses the board revision to return a numeric value representing
>> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
>>
>> Knowing the Pi family will help us set the SD card routing when we
>> introduce support for the Pi 4 and should also be easier to maintain
>> than if using individual model detection.
>>
>> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
>> RpiFirmwareGetModelName ().
> 
> Can you drop the above line and include the below as 1/? in v2?

Okay.

Note that since you requested alphabetical for PCDs, I'm going to have 
an "Also" in 2/ (now 3/) since the existing PCDs in 
Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of 
alphabetical order.

I sure hope you're not going to ask me to split this extra reordering 
into a separate commit...

Regards,

/Pete

> 
> /
>      Leif
> 
>  From 59f01ff36ac7918e9ce166acbd3e963f638ab4b1 Mon Sep 17 00:00:00 2001
> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
> Date: Mon, 18 Nov 2019 17:47:06 +0000
> Subject: [PATCH edk2-platforms 1/1] Platform/RPi: Add missing model name
> 
> add a missing entry for the "Raspberry Pi Compute Module 3+" in
> RpiFirmwareGetModelName ().
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>   Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> index 9b4aa068857c..dcb434fabefe 100644
> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> @@ -1,5 +1,6 @@
>   /** @file
>    *
> + *  Copyright (c) 2019, ARM Limited. All rights reserved.
>    *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>    *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
>    *
> @@ -595,6 +596,8 @@ RpiFirmwareGetModelName (
>       return "Raspberry Pi 3 Model B+";
>     case 0x0E:
>       return "Raspberry Pi 3 Model A+";
> +  case 0x10:
> +    return "Raspberry Pi Compute Module 3+";
>     case 0x11:
>       return "Raspberry Pi 4 Model B";
>     default:
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50847): https://edk2.groups.io/g/devel/message/50847
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Nov 18, 2019 at 05:58:05PM +0000, Pete Batard wrote:
> On 2019.11.18 17:51, Leif Lindholm wrote:
> > On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
> > > From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
> > > 
> > > Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
> > > 
> > > This uses the board revision to return a numeric value representing
> > > the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
> > > 
> > > Knowing the Pi family will help us set the SD card routing when we
> > > introduce support for the Pi 4 and should also be easier to maintain
> > > than if using individual model detection.
> > > 
> > > Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
> > > RpiFirmwareGetModelName ().
> > 
> > Can you drop the above line and include the below as 1/? in v2?
> 
> Okay.
> 
> Note that since you requested alphabetical for PCDs, I'm going to have an
> "Also" in 2/ (now 3/) since the existing PCDs in
> Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of
> alphabetical order.

Actually, I try to never request reordering of existing lines, so I
would be quite happy for you to skip the changes that would motivate
the use of the "also".

I tend to apply a rule of trying to insert *new* (or moved) lines in a
way that will improve the existing order - or in messy cases at least
not make it worse.

I have had it pointed out to me that this is maybe not entirely
obvious...

Regards,

Leif

> I sure hope you're not going to ask me to split this extra reordering into a
> separate commit...
> 
> Regards,
> 
> /Pete
> 
> > 
> > /
> >      Leif
> > 
> >  From 59f01ff36ac7918e9ce166acbd3e963f638ab4b1 Mon Sep 17 00:00:00 2001
> > From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
> > Date: Mon, 18 Nov 2019 17:47:06 +0000
> > Subject: [PATCH edk2-platforms 1/1] Platform/RPi: Add missing model name
> > 
> > add a missing entry for the "Raspberry Pi Compute Module 3+" in
> > RpiFirmwareGetModelName ().
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >   Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> > index 9b4aa068857c..dcb434fabefe 100644
> > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> > @@ -1,5 +1,6 @@
> >   /** @file
> >    *
> > + *  Copyright (c) 2019, ARM Limited. All rights reserved.
> >    *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> >    *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> >    *
> > @@ -595,6 +596,8 @@ RpiFirmwareGetModelName (
> >       return "Raspberry Pi 3 Model B+";
> >     case 0x0E:
> >       return "Raspberry Pi 3 Model A+";
> > +  case 0x10:
> > +    return "Raspberry Pi Compute Module 3+";
> >     case 0x11:
> >       return "Raspberry Pi 4 Model B";
> >     default:
> > 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50848): https://edk2.groups.io/g/devel/message/50848
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2019.11.18 18:05, Leif Lindholm wrote:
> On Mon, Nov 18, 2019 at 05:58:05PM +0000, Pete Batard wrote:
>> On 2019.11.18 17:51, Leif Lindholm wrote:
>>> On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
>>>> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
>>>>
>>>> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
>>>>
>>>> This uses the board revision to return a numeric value representing
>>>> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
>>>>
>>>> Knowing the Pi family will help us set the SD card routing when we
>>>> introduce support for the Pi 4 and should also be easier to maintain
>>>> than if using individual model detection.
>>>>
>>>> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
>>>> RpiFirmwareGetModelName ().
>>>
>>> Can you drop the above line and include the below as 1/? in v2?
>>
>> Okay.
>>
>> Note that since you requested alphabetical for PCDs, I'm going to have an
>> "Also" in 2/ (now 3/) since the existing PCDs in
>> Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of
>> alphabetical order.
> 
> Actually, I try to never request reordering of existing lines, so I
> would be quite happy for you to skip the changes that would motivate
> the use of the "also".
> 
> I tend to apply a rule of trying to insert *new* (or moved) lines in a
> way that will improve the existing order - or in messy cases at least
> not make it worse.
> 
> I have had it pointed out to me that this is maybe not entirely
> obvious...

Well, this is exactly what I would point out as an example of the strive 
for commit atomicity getting in the way of a more readable codebase as 
well as overall user experience (the users here being the developers who 
are dealing with the code). The reason I'm pointing this out is that, in 
the past, I have been dealing with projects that seemed to care more 
about keeping a squeaky clean commit history than they seemed to care 
about making the underlying code as good as it could possibly get, which 
resulted in increased pain for the developers having to contend with 
said codebase and ultimately end-users of the software produced from 
that codebase.

Again, I would assert that there has to exist a middle ground between 
keeping a super-clean commit history and improving the source where it 
can indeed be improved at little cost, by not always defaulting to 
people having to devote extra time splitting patches.

But I understand this is not my choice to make here. Thus I'll stay away 
from reordering that doesn't have to do with new PCDs being introduced.

Regards,

/Pete

> 
> Regards,
> 
> Leif
> 
>> I sure hope you're not going to ask me to split this extra reordering into a
>> separate commit...
>>
>> Regards,
>>
>> /Pete
>>
>>>
>>> /
>>>       Leif
>>>
>>>   From 59f01ff36ac7918e9ce166acbd3e963f638ab4b1 Mon Sep 17 00:00:00 2001
>>> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
>>> Date: Mon, 18 Nov 2019 17:47:06 +0000
>>> Subject: [PATCH edk2-platforms 1/1] Platform/RPi: Add missing model name
>>>
>>> add a missing entry for the "Raspberry Pi Compute Module 3+" in
>>> RpiFirmwareGetModelName ().
>>>
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>    Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> index 9b4aa068857c..dcb434fabefe 100644
>>> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> @@ -1,5 +1,6 @@
>>>    /** @file
>>>     *
>>> + *  Copyright (c) 2019, ARM Limited. All rights reserved.
>>>     *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>>>     *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
>>>     *
>>> @@ -595,6 +596,8 @@ RpiFirmwareGetModelName (
>>>        return "Raspberry Pi 3 Model B+";
>>>      case 0x0E:
>>>        return "Raspberry Pi 3 Model A+";
>>> +  case 0x10:
>>> +    return "Raspberry Pi Compute Module 3+";
>>>      case 0x11:
>>>        return "Raspberry Pi 4 Model B";
>>>      default:
>>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50849): https://edk2.groups.io/g/devel/message/50849
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, 18 Nov 2019 at 19:32, Pete Batard <pete@akeo.ie> wrote:
>
> On 2019.11.18 18:05, Leif Lindholm wrote:
> > On Mon, Nov 18, 2019 at 05:58:05PM +0000, Pete Batard wrote:
> >> On 2019.11.18 17:51, Leif Lindholm wrote:
> >>> On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
> >>>> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
> >>>>
> >>>> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
> >>>>
> >>>> This uses the board revision to return a numeric value representing
> >>>> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
> >>>>
> >>>> Knowing the Pi family will help us set the SD card routing when we
> >>>> introduce support for the Pi 4 and should also be easier to maintain
> >>>> than if using individual model detection.
> >>>>
> >>>> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
> >>>> RpiFirmwareGetModelName ().
> >>>
> >>> Can you drop the above line and include the below as 1/? in v2?
> >>
> >> Okay.
> >>
> >> Note that since you requested alphabetical for PCDs, I'm going to have an
> >> "Also" in 2/ (now 3/) since the existing PCDs in
> >> Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of
> >> alphabetical order.
> >
> > Actually, I try to never request reordering of existing lines, so I
> > would be quite happy for you to skip the changes that would motivate
> > the use of the "also".
> >
> > I tend to apply a rule of trying to insert *new* (or moved) lines in a
> > way that will improve the existing order - or in messy cases at least
> > not make it worse.
> >
> > I have had it pointed out to me that this is maybe not entirely
> > obvious...
>
> Well, this is exactly what I would point out as an example of the strive
> for commit atomicity getting in the way of a more readable codebase as
> well as overall user experience (the users here being the developers who
> are dealing with the code). The reason I'm pointing this out is that, in
> the past, I have been dealing with projects that seemed to care more
> about keeping a squeaky clean commit history than they seemed to care
> about making the underlying code as good as it could possibly get, which
> resulted in increased pain for the developers having to contend with
> said codebase and ultimately end-users of the software produced from
> that codebase.
>
> Again, I would assert that there has to exist a middle ground between
> keeping a super-clean commit history and improving the source where it
> can indeed be improved at little cost, by not always defaulting to
> people having to devote extra time splitting patches.
>
> But I understand this is not my choice to make here. Thus I'll stay away
> from reordering that doesn't have to do with new PCDs being introduced.
>

Please keep in mind that when open source maintainers take ownership
of your code, they assume the responsibility to ensure that it doesn't
get broken by future updates elsewhere in the codebase, often way
beyond the commercial lifetime of the product that is supported by
that code. This is a sizable effort, and an important part of managing
that effort is ensuring that the code is in an acceptable shape to
begin with, and what 'acceptable' means differs between different
maintainers. Not being able to revert a patch easily because it
touches unrelated code may make our lives more difficult years after
you have stopped caring about this platform entirely.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50907): https://edk2.groups.io/g/devel/message/50907
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2019.11.19 15:07, Ard Biesheuvel wrote:
> On Mon, 18 Nov 2019 at 19:32, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2019.11.18 18:05, Leif Lindholm wrote:
>>> On Mon, Nov 18, 2019 at 05:58:05PM +0000, Pete Batard wrote:
>>>> On 2019.11.18 17:51, Leif Lindholm wrote:
>>>>> On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote:
>>>>>> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
>>>>>>
>>>>>> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL.
>>>>>>
>>>>>> This uses the board revision to return a numeric value representing
>>>>>> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4).
>>>>>>
>>>>>> Knowing the Pi family will help us set the SD card routing when we
>>>>>> introduce support for the Pi 4 and should also be easier to maintain
>>>>>> than if using individual model detection.
>>>>>>
>>>>>> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in
>>>>>> RpiFirmwareGetModelName ().
>>>>>
>>>>> Can you drop the above line and include the below as 1/? in v2?
>>>>
>>>> Okay.
>>>>
>>>> Note that since you requested alphabetical for PCDs, I'm going to have an
>>>> "Also" in 2/ (now 3/) since the existing PCDs in
>>>> Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of
>>>> alphabetical order.
>>>
>>> Actually, I try to never request reordering of existing lines, so I
>>> would be quite happy for you to skip the changes that would motivate
>>> the use of the "also".
>>>
>>> I tend to apply a rule of trying to insert *new* (or moved) lines in a
>>> way that will improve the existing order - or in messy cases at least
>>> not make it worse.
>>>
>>> I have had it pointed out to me that this is maybe not entirely
>>> obvious...
>>
>> Well, this is exactly what I would point out as an example of the strive
>> for commit atomicity getting in the way of a more readable codebase as
>> well as overall user experience (the users here being the developers who
>> are dealing with the code). The reason I'm pointing this out is that, in
>> the past, I have been dealing with projects that seemed to care more
>> about keeping a squeaky clean commit history than they seemed to care
>> about making the underlying code as good as it could possibly get, which
>> resulted in increased pain for the developers having to contend with
>> said codebase and ultimately end-users of the software produced from
>> that codebase.
>>
>> Again, I would assert that there has to exist a middle ground between
>> keeping a super-clean commit history and improving the source where it
>> can indeed be improved at little cost, by not always defaulting to
>> people having to devote extra time splitting patches.
>>
>> But I understand this is not my choice to make here. Thus I'll stay away
>> from reordering that doesn't have to do with new PCDs being introduced.
>>
> 
> Please keep in mind that when open source maintainers take ownership
> of your code, they assume the responsibility to ensure that it doesn't
> get broken by future updates elsewhere in the codebase, often way
> beyond the commercial lifetime of the product that is supported by
> that code. This is a sizable effort, and an important part of managing
> that effort is ensuring that the code is in an acceptable shape to
> begin with, and what 'acceptable' means differs between different
> maintainers. Not being able to revert a patch easily because it
> touches unrelated code may make our lives more difficult years after
> you have stopped caring about this platform entirely.

I think you are actually exposing the root of the problem without 
realizing it here.

Elements that may make a maintainer's life more difficult years after 
the contributor stopped working on it can actually be elements that 
makes, and will continue to make, a whole lot of developers' lives much 
easier right now.

For instance, someone today or tomorrow (rather than 2 or 5 years down 
the line) can very well copy from code that got rejected as an "Also" 
(say, the one instance I found in the Pi source where a %s was used 
instead of a %a, which is an easy thing to miss if you're not paying 
attention) and find out they are wasting time on an issue that they 
would never have had to contend with, had the EDK2 maintainership been 
flexible with regards to what might be acceptable to piggyback on a 
patch that pertains to a specific file (IMO, fixing typos or style 
should always be acceptable as a piggyback, and I'd really like to hear 
how including such changes is effectively going to make the maintainers' 
job that harder down the line).

And though this is a not directly related issue, I could also speak 
volumes on how myself, and I assume many, many other developers, have 
wasted countless hours (my current estimate puts that to around 4 to 5 
hours in my case) on the current CRLF enforcing situation with the EDK2 
codebase.

All this to re-state that I wish there existed a balance between the 
well established needs of the maintainers, and what they envision might 
emerge as issues in the long run (which I assert tends to encourage them 
to preserve an existing status-quo), and the possibly not so well 
publicized pain points and time wastage that consumers of the codebase 
encounter, who, of course (and, depending on how this discussion goes, I 
might come to see as perhaps the wisest choice) generally tend to avoid 
venting their frustration on a mailing list that aims at concerning 
itself solely with technical discussions...

In other words, if you are willing to consider how much more painful 
allowing the piggybacking of low-hanging "Also"'s onto existing patch 
may make your life as a maintainer down the line, please also be willing 
to envision the scenarios in which not allowing the same thing might 
actually be making the life of people who work with the codebase, and 
I'd really like to stress out that I'm really not talking only about 
myself here, harder right now.

Regards,

/Pete

> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50910): https://edk2.groups.io/g/devel/message/50910
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Nov 19, 2019 at 04:30:05PM +0000, Pete Batard wrote:
> > Please keep in mind that when open source maintainers take ownership
> > of your code, they assume the responsibility to ensure that it doesn't
> > get broken by future updates elsewhere in the codebase, often way
> > beyond the commercial lifetime of the product that is supported by
> > that code. This is a sizable effort, and an important part of managing
> > that effort is ensuring that the code is in an acceptable shape to
> > begin with, and what 'acceptable' means differs between different
> > maintainers. Not being able to revert a patch easily because it
> > touches unrelated code may make our lives more difficult years after
> > you have stopped caring about this platform entirely.
>
> I think you are actually exposing the root of the problem without realizing
> it here.

That is quite a condescending thing to say.

> Elements that may make a maintainer's life more difficult years after the
> contributor stopped working on it can actually be elements that makes, and
> will continue to make, a whole lot of developers' lives much easier right
> now.

Much easier than occasionally using git add --patch or git-gui to
stage individual hunks?

Splitting occasional minor changes out into separate patches should be
< 1min effort.

> For instance, someone today or tomorrow (rather than 2 or 5 years down the
> line) can very well copy from code that got rejected as an "Also" (say, the
> one instance I found in the Pi source where a %s was used instead of a %a,
> which is an easy thing to miss if you're not paying attention) and find out
> they are wasting time on an issue that they would never have had to contend
> with, had the EDK2 maintainership been flexible with regards to what might
> be acceptable to piggyback on a patch that pertains to a specific file (IMO,
> fixing typos or style should always be acceptable as a piggyback, and I'd
> really like to hear how including such changes is effectively going to make
> the maintainers' job that harder down the line).

Ard gave a very specific example in the email you are replying to.

I can give (and have given) you others, but since those have seen no
reaction (either acknowledgment or detraction) from you, there seems
to be little point in adding more.

> And though this is a not directly related issue, I could also speak volumes
> on how myself, and I assume many, many other developers, have wasted
> countless hours (my current estimate puts that to around 4 to 5 hours in my
> case) on the current CRLF enforcing situation with the EDK2 codebase.

That is a completely unrelated issue, which I have certainly also
wasted spectacular amounts of time on. And am working towards getting
rid of.

> All this to re-state that I wish there existed a balance between the well
> established needs of the maintainers, and what they envision might emerge as
> issues in the long run (which I assert tends to encourage them to preserve
> an existing status-quo), and the possibly not so well publicized pain points
> and time wastage that consumers of the codebase encounter, who, of course
> (and, depending on how this discussion goes, I might come to see as perhaps
> the wisest choice) generally tend to avoid venting their frustration on a
> mailing list that aims at concerning itself solely with technical
> discussions...

This isn't a balance discussion. Either you believe that open source
development happens in changesets or you do not. Either you see the
value in that for debugging, or you do not.

If what you care about is the ability to go back to what the tree
looked like at a given point in time, then sure, a lot of this will
seem very tedious to you.

This does not mean that any amount of debating the topic will convince
anyone who relies on the fundamentality of changesets for their
workflow.

> In other words, if you are willing to consider how much more painful
> allowing the piggybacking of low-hanging "Also"'s onto existing patch may
> make your life as a maintainer down the line, please also be willing to
> envision the scenarios in which not allowing the same thing might actually
> be making the life of people who work with the codebase, and I'd really like
> to stress out that I'm really not talking only about myself here, harder
> right now.

You do realise that apart from reviewing patches, we also write and
contribute code ourselves - including to several other projects?
All which follow the exact same rule.

Suffice to say, this aspect of TianoCore is no more negotiable than
the same aspect of linux, u-boot or QEMU.

I will be sorry to see you stop contributing to TianoCore if that is
the effect, but I am not willing to continue to rehash the very
fundamentals of open source development.

/

    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50935): https://edk2.groups.io/g/devel/message/50935
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Leif,

On 2019.11.20 10:27, Leif Lindholm wrote:
> On Tue, Nov 19, 2019 at 04:30:05PM +0000, Pete Batard wrote:
>>> Please keep in mind that when open source maintainers take ownership
>>> of your code, they assume the responsibility to ensure that it doesn't
>>> get broken by future updates elsewhere in the codebase, often way
>>> beyond the commercial lifetime of the product that is supported by
>>> that code. This is a sizable effort, and an important part of managing
>>> that effort is ensuring that the code is in an acceptable shape to
>>> begin with, and what 'acceptable' means differs between different
>>> maintainers. Not being able to revert a patch easily because it
>>> touches unrelated code may make our lives more difficult years after
>>> you have stopped caring about this platform entirely.
>>
>> I think you are actually exposing the root of the problem without realizing
>> it here.
> 
> That is quite a condescending thing to say.
> 
>> Elements that may make a maintainer's life more difficult years after the
>> contributor stopped working on it can actually be elements that makes, and
>> will continue to make, a whole lot of developers' lives much easier right
>> now.
> 
> Much easier than occasionally using git add --patch or git-gui to
> stage individual hunks?
> 
> Splitting occasional minor changes out into separate patches should be
> < 1min effort.

That "should" *is* the exact issue.

That is the very core of what I am trying to raise here: additional time 
that is being incurred on contributors (as well as maintainers), which, 
I will continue to assert, if a project was less striving for academia 
excellence and more for the real life preservation of everyone's limited 
resources, it should be acceptable to spare when my own experience with 
both contributing and maintaining Open Source projects (yes, most of 
them not as large as EDK2, but also most of them not as small as a a 
single person's side project) has repeatedly demonstrated that 
flexibility rather than intractability with regards to rules, when they 
happen to save everybody's time, is one of the greatest factor in 
attracting quality contributions.

The root of the matter is that, more than often, I have found that what 
"should" be a <1 min effort, turns into a 15 mins or in the worst case 
up to a 1 hour endeavour due to side issues stepping in. And yes, that 
applies even to splitting the occasional minor changes for one-liner 
typos (and please don't be tempted to construe these as unfamiliarity 
with the tools being used), though I will agree that, in most cases, it 
*shouldn't* be that bad. But I will firmly dispute the idea that even 
the most straightforward typo fix effectively takes less than a minute 
to accomplish when you are considering the whole picture (rather than 
the amount of time one may effectively be spending invoking git commands).

Or are you under the impression that someone who is advocating for patch 
piggybacking is not concerned enough with the quality of what they 
submit, to actually want to validate that what they are splitting is up 
to good standards? Even for a one liner, ensuring that a split commit 
does comply to the expected standards of a project like EDK2 is more 
than a one minute affair. And everybody who has enough experience is 
familiar with a "Surely this simple one liner shouldn't break anything" 
that turned into a more dramatic affair than wanted. And yes, I am also 
considering the case where one would be doing the split from the get go 
rather than the one where one submitted a patchset and then a maintainer 
asked them to further split part of it.

Therefore, if you are thinking the earlier statement you pointed to 
above was condescending, then I'd like to present another contender for 
that prize. This <1 min effort is not the reality I have been observing, 
or the one I am expecting anyone else to be observing, which is what is 
actually prompting me to want to go over these matters.

Instead, I will assert that spending less than a minute splitting minor 
changes into a patch series is the exception rather than the rule for 
any contributor who does take the quality of what they submit seriously.

And that is the very core of the issue, because it then becomes a matter 
of having to deallocate time from other endeavours, which is what I 
would like to see at least acknowledged instead of being dismissed as a 
"non issue", because, IMO, this is a matter that I think Open Source 
project maintainers also need to keep on their radar.

>> For instance, someone today or tomorrow (rather than 2 or 5 years down the
>> line) can very well copy from code that got rejected as an "Also" (say, the
>> one instance I found in the Pi source where a %s was used instead of a %a,
>> which is an easy thing to miss if you're not paying attention) and find out
>> they are wasting time on an issue that they would never have had to contend
>> with, had the EDK2 maintainership been flexible with regards to what might
>> be acceptable to piggyback on a patch that pertains to a specific file (IMO,
>> fixing typos or style should always be acceptable as a piggyback, and I'd
>> really like to hear how including such changes is effectively going to make
>> the maintainers' job that harder down the line).
> 
> Ard gave a very specific example in the email you are replying to.

I believe Ard gave a conditional example of what he thinks may happen 
down the line.

I tried to counter with the idea that, if you try to look at the big 
picture, maybe the time possibly being spent that far down the line is 
going to be more than compensated by the time having already been saved 
by others.

> I can give (and have given) you others, but since those have seen no
> reaction (either acknowledgment or detraction) from you, there seems
> to be little point in adding more.
> 
>> And though this is a not directly related issue, I could also speak volumes
>> on how myself, and I assume many, many other developers, have wasted
>> countless hours (my current estimate puts that to around 4 to 5 hours in my
>> case) on the current CRLF enforcing situation with the EDK2 codebase.
> 
> That is a completely unrelated issue, which I have certainly also
> wasted spectacular amounts of time on.

I wouldn't say completely unrelated. Though I am of course not aware of 
the full specifics of it, which might justify some of the delayed action 
that is being taken. Instead, I was trying to use it as an example of a 
project seemingly deciding that it should be acceptable for contributors 
to collectively waste what I will assert is an exceedingly large amount 
of time, rather than just bite the bullet and state "We're gonna pause 
everything and take how many number of days we need to fix that now, so 
that our contributors stop having to waste hours on a matter that no 
other major Open Source project has to make them contend with".

Considering that you are happy to cite the Linux kernel as an example, 
let me posit the following: Do you believe that Linus would have just 
acknowledged that the CRLF issue as something that it is acceptable to 
fix years down the line and that contributors should just have to learn 
to accept for however long it may remain unaddressed?

And that is precisely my point: How far does a project see it as 
acceptable to push time wastage onto contributors, be it for having to 
contend with CRLF issues or spending an extra 5, 10, 15 minutes here and 
there (sometimes more) to split couple-liners into separate patches.

This, IMO, makes the CRLF matter related to this whole discussion. As a 
matter of fact, it is in part not seeing the matter of CRLF having been 
addressed a long time ago that lead me to worry enough about the 
direction this project wants to take to want to post about patch 
piggybacking for typos and style. Had EDK2 sorted CRLF, I probably would 
have kept this reflection to myself...

> And am working towards getting rid of.

That's good to hear. It genuinely can't happen fast enough as I've 
literally just wasted about another hour on this today...

>> All this to re-state that I wish there existed a balance between the well
>> established needs of the maintainers, and what they envision might emerge as
>> issues in the long run (which I assert tends to encourage them to preserve
>> an existing status-quo), and the possibly not so well publicized pain points
>> and time wastage that consumers of the codebase encounter, who, of course
>> (and, depending on how this discussion goes, I might come to see as perhaps
>> the wisest choice) generally tend to avoid venting their frustration on a
>> mailing list that aims at concerning itself solely with technical
>> discussions...
> 
> This isn't a balance discussion. Either you believe that open source
> development happens in changesets or you do not.

I'm just trying to share my experience, stemmed from participating in 
(as well as maintaining) multiple Open Source projects, some of which 
worked well and others which I would qualify as more dysfunctional, 
where I have found that intractability in changeset rules, and the 
inability of maintainers to acknowledge collective "time wastage" (by 
insisting that rules are not there to be flexible or even debated) 
eventually became a deterrent to attracting contributions and, in some 
case, resulted in the project being left struggling.

And please understand that I'm not pushing for maintainership to just 
accept whatever (for instance, I'm not going to state that a patch that 
contains 4 lines of effective code change and 4 of fixing typos/style, 
even super obvious one, should not be split). Just that there should be 
some form of balance with regards to how flexible patchset rules should 
be when one can expect that the collective amount of time wasted by both 
maintainership and contributorship can be reduced in the long run 
(which, in the absolute, may indeed very well incur additional time 
being spent by maintainership, but which, and this is my point, 
maintainership should understand is part of the collective effort to 
ensure that one of the prime resource they need to concern themselves 
with, i.e. the time that needs to be devoted to achieve a desired 
standard of code quality, is minimized for everyone rather than just a 
single party).

> Either you see the
> value in that for debugging, or you do not.
> 
> If what you care about is the ability to go back to what the tree
> looked like at a given point in time, then sure, a lot of this will
> seem very tedious to you.
> 
> This does not mean that any amount of debating the topic will convince
> anyone who relies on the fundamentality of changesets for their
> workflow.
> 
>> In other words, if you are willing to consider how much more painful
>> allowing the piggybacking of low-hanging "Also"'s onto existing patch may
>> make your life as a maintainer down the line, please also be willing to
>> envision the scenarios in which not allowing the same thing might actually
>> be making the life of people who work with the codebase, and I'd really like
>> to stress out that I'm really not talking only about myself here, harder
>> right now.
> 
> You do realise that apart from reviewing patches, we also write and
> contribute code ourselves - including to several other projects?
> All which follow the exact same rule.

Yes. And I am trying to make a point for the collective, which includes 
not just you, but everyone involved in the project.

> Suffice to say, this aspect of TianoCore is no more negotiable than
> the same aspect of linux, u-boot or QEMU.

Yes, that is entirely the choice of the maintainership of a project.

Which is why I am trying to invite them to consider one aspect that I 
believe is often overlooked: trying to treat time as the 3d most 
valuable resource a project needs to concern itself with (end-user 
experience being first and overall code/software quality second), and 
applying flexibility to what some might be a bit too eager to treat as 
non-negotiable rules as a result of that. Rules should be made to serve 
and foster those resources rather than the opposite.

Still I don't expect the project, or more exactly, individual attitudes 
with regards to what one sees as beneficial to the project, to change 
overnight, especially for a behemoth like EDK2. But I want to leave that 
point in the open so that people like you can at least reflect on it 
when they are confronted with various aspects of the project's life, 
which *may* eventually lead them to rethink, as it happened to me, 
whether *some* amount of flexibility can actually be more beneficial to 
a project than over reliance on intractable rules.

> I will be sorry to see you stop contributing to TianoCore if that is
> the effect,

I'm not planning to. As a matter of fact, some years ago, I might have 
made the same arguments you are making to dismiss my very own point, as 
it's only from participating in multiple projects that I have come to 
realize the importance of minimizing time wastage when it comes to 
keeping a project attractive. So I don't consider this discussion, 
regardless of its outcome, as something that would ever lead me to quit 
wanting to contribute to EDK2 altogether. And I also certainly hope that 
this discussion does not come across as as me berating maintainership of 
the project, which, when compared to other projects, I think is actually 
doing a very fine job overall (even if I may come to disagree with some 
the more minute aspects of what is for best when it comes to maintaining 
a project).

It will however limit my willingness to fix low-hanging fruits, as I 
can't recall instances where ones I've tried to fix in the past were the 
<1 min effort you talk about. Even the patch you sent where you 
effectively did the splitting ended up being a 10-15 mins job to 
integrate and resulted in me having to manually edit stuff. And that is 
not even taking into account the time incurred by having to rework the 
second patch in the series or work needed to get the repo to a state 
where said patch should have been applicable.

> but I am not willing to continue to rehash the very
> fundamentals of open source development.

Open Source is what we make it. I believe it is still new enough to 
still be trying to figure out the "rules" that work best. I certainly 
wouldn't say we have figured everything yet to be sure that some of the 
rules we currently apply don't warrant further tweaking...

Regards,

/Pete

PS: If folks start to think that discussion is becoming too distracting 
for the list, then I have no problem taking it off-list, tough I would 
assert that I've pretty much covered everything I wanted to expose with 
regards to my position at this stage.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#50984): https://edk2.groups.io/g/devel/message/50984
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 11/20/19 22:50, Pete Batard wrote:

> [...]
> 
> Which is why I am trying to invite them to consider one aspect that I
> believe is often overlooked: trying to treat time as the 3d most
> valuable resource a project needs to concern itself with (end-user
> experience being first and overall code/software quality second), and
> applying flexibility to what some might be a bit too eager to treat as
> non-negotiable rules as a result of that. Rules should be made to serve
> and foster those resources rather than the opposite.

Contribution rules are already made to prioritize time and effort --
*maintainer* time and effort.

- There are fewer maintainers than contributors.

- Maintainers tend to stick around for long, contributors may or may not
(it varies).

- Maintainers generally take more responsibility for the codebase, as a
whole, than contributors do.

- In most cases, reading code is more difficult than writing code.

All of the above turn maintainership and patch review into a permanent
bottleneck at the project level. Unclogging that bottleneck is what
project rules prioritize.

Nobody doubts that strict contribution rules create bottlenecks on the
contributor side. That's the lesser wrong. "Moving fast" leads to
regressions. In a halfway mature project, which users have grown to rely
on, regressions destroy end-user experience (which you put as first
priority).

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51008): https://edk2.groups.io/g/devel/message/51008
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 11/21/19 09:55, Laszlo Ersek wrote:
> On 11/20/19 22:50, Pete Batard wrote:
> 
>> [...]
>>
>> Which is why I am trying to invite them to consider one aspect that I
>> believe is often overlooked: trying to treat time as the 3d most
>> valuable resource a project needs to concern itself with (end-user
>> experience being first and overall code/software quality second), and
>> applying flexibility to what some might be a bit too eager to treat as
>> non-negotiable rules as a result of that. Rules should be made to serve
>> and foster those resources rather than the opposite.
> 
> Contribution rules are already made to prioritize time and effort --
> *maintainer* time and effort.
> 
> - There are fewer maintainers than contributors.
> 
> - Maintainers tend to stick around for long, contributors may or may not
> (it varies).
> 
> - Maintainers generally take more responsibility for the codebase, as a
> whole, than contributors do.
> 
> - In most cases, reading code is more difficult than writing code.
> 
> All of the above turn maintainership and patch review into a permanent
> bottleneck at the project level. Unclogging that bottleneck is what
> project rules prioritize.
> 
> Nobody doubts that strict contribution rules create bottlenecks on the
> contributor side. That's the lesser wrong. "Moving fast" leads to
> regressions. In a halfway mature project, which users have grown to rely
> on, regressions destroy end-user experience (which you put as first
> priority).

BTW I don't claim that "strict rules" are the only way to keep
regressions out.

Many projects that "move fast" justify moving fast, and justify easing
up on patch review, with having thorough CI. "Thorough CI" is also not
easy though (in particular integration tests) -- keeping the test suite
up-to-date, reviewing patches for the tests (incl. test infrastructure),
plus operating the test environment, also require time and effort.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51025): https://edk2.groups.io/g/devel/message/51025
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

I appreciate your input on this.

However, I find it unfortunate that you seem to be equating my concern 
about minimizing time wastage to "moving fast", as those are two very 
different matters, and "moving fast" isn't what I have been trying to 
advocate for at all.

None of the points I tried to raise are borne from what you might 
perhaps have perceived as a contributor's frustration about a project 
not moving fast enough in their eye. Especially, considering that I've 
literally had to deal with Open Source projects where integration of 
some patches became a multi-year affair (and where this extended delay 
had nothing to do with the usual review/rework/resubmit process) then I 
have to state that there is little to be frustrated about when it comes 
to how fast one is able to get things reviewed and integrated in the EDK2.

Yet, one can move fast or slow, and still waste time.

I guess if I wanted to address some of the points you raise, I could 
mention that lack of flexibility with regards to some rules can come as 
a deterrent to contributors, especially new ones if they have to spend 
what they perceive as an unwarranted amount of their time on elements 
that they'll be hard pressed to see actual tangible benefits of for the 
project as a whole, and how this may ultimately play into contributors 
not wanting to stick around (with the end result of increased work and 
wasted time for maintainers).

But I feel like I would mostly be a re-hashing of what I have already 
tried to point out. Therefore, with the clarification above having been 
made, I am planning to leave the matter at that.

Regards,

/Pete

On 2019.11.21 09:04, Laszlo Ersek wrote:
> On 11/21/19 09:55, Laszlo Ersek wrote:
>> On 11/20/19 22:50, Pete Batard wrote:
>>
>>> [...]
>>>
>>> Which is why I am trying to invite them to consider one aspect that I
>>> believe is often overlooked: trying to treat time as the 3d most
>>> valuable resource a project needs to concern itself with (end-user
>>> experience being first and overall code/software quality second), and
>>> applying flexibility to what some might be a bit too eager to treat as
>>> non-negotiable rules as a result of that. Rules should be made to serve
>>> and foster those resources rather than the opposite.
>>
>> Contribution rules are already made to prioritize time and effort --
>> *maintainer* time and effort.
>>
>> - There are fewer maintainers than contributors.
>>
>> - Maintainers tend to stick around for long, contributors may or may not
>> (it varies).
>>
>> - Maintainers generally take more responsibility for the codebase, as a
>> whole, than contributors do.
>>
>> - In most cases, reading code is more difficult than writing code.
>>
>> All of the above turn maintainership and patch review into a permanent
>> bottleneck at the project level. Unclogging that bottleneck is what
>> project rules prioritize.
>>
>> Nobody doubts that strict contribution rules create bottlenecks on the
>> contributor side. That's the lesser wrong. "Moving fast" leads to
>> regressions. In a halfway mature project, which users have grown to rely
>> on, regressions destroy end-user experience (which you put as first
>> priority).
> 
> BTW I don't claim that "strict rules" are the only way to keep
> regressions out.
> 
> Many projects that "move fast" justify moving fast, and justify easing
> up on patch review, with having thorough CI. "Thorough CI" is also not
> easy though (in particular integration tests) -- keeping the test suite
> up-to-date, reviewing patches for the tests (incl. test infrastructure),
> plus operating the test environment, also require time and effort.
> 
> Thanks
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51111): https://edk2.groups.io/g/devel/message/51111
Mute This Topic: https://groups.io/mt/57792459/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-