Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model

["Pete Batard" <pete@akeo.ie>]

On 2019.11.27 15:24, Philippe Mathieu-Daudé wrote:
> On 11/27/19 1:37 PM, Pete Batard wrote:
>> From: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
>>
>> The Raspberry Pi 4 has a new SD controller. As a result we must
>> handle SD routing according to the model, which we perform in
>> the Config driver by using the GetModelFamily () call that was
>> recently introduced.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137 
>> ++++++++++++++------
>>   1 file changed, 96 insertions(+), 41 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index 98e58a560ed4..26bc92f28185 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -1,6 +1,7 @@
>>   /** @file
>>    *
>> - *  Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>> + *  Copyright (c) 2019, ARM Limited. All rights reserved.
> 
> "All rights reserved."?

To be honest, that's something that's been bothering me too in this 
codebase (and some other ones too, where you get to see the same), since 
there are only so many rights one can reserve when the code is actually 
governed by the Open Source license being used, and therefore asserting 
that you reserve "all rights" seems to be in direct conflict with that.

However, I am not a lawyer, and this seems to be standard boilerplate 
being imposed by large companies. For instance, you'll find plenty of 
instances of it in the existing codebase. E.g. 
https://github.com/tianocore/edk2/blob/master/ArmPkg/Include/AsmMacroIoLib.h 
has three separate entities that appear to state that each one holds all 
the rights to the source, which I can't help by find amusing.

I guess we're supposed to understand that each entity reserves all 
rights to the code they've actually written (including the right to do 
something that might go against the license, since "All rights" > 
"Rights to the extent being granted by the BSD"), and that it's up to 
legal departments to sort up the mess, if mess there is...

Then again, while I think I can wrap my head against what copyright 
entails, I'm not sure I completely get what these additional "rights" 
are supposed to mean in this context (my current take being that we're 
supposed to be believe that there exists an implicit grandfathered 
license, which gives all rights to the parent company, and that governs 
a virtual version of the source code containing only the changes that 
the developer applied, and therefore that the BSD licensed version of 
the source that is then made public is meant to be seen as a derivative 
of this virtual "All rights reserved" incomplete source, hence granting 
a partial "All rights" for said source to the company, if that makes any 
sense), so it may be good for someone with better understanding of this 
to clarify, or point to a place where this might be explained.

> 
>> + *  Copyright (c) 2018 - 2019, Andrei Warkentin 
>> <andrey.warkentin@gmail.com>
>>    *
>>    *  SPDX-License-Identifier: BSD-2-Clause-Patent
>>    *
>> @@ -9,10 +10,12 @@
>>   #include <Uefi.h>
>>   #include <Library/HiiLib.h>
>>   #include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>>   #include <Library/UefiBootServicesTableLib.h>
>>   #include <Library/UefiRuntimeServicesTableLib.h>
>>   #include <Library/DevicePathLib.h>
>>   #include <IndustryStandard/RpiMbox.h>
>> +#include <IndustryStandard/Bcm2836.h>
>>   #include <IndustryStandard/Bcm2836Gpio.h>
>>   #include <Library/GpioLib.h>
>>   #include <Protocol/RpiFirmware.h>
>> @@ -212,6 +215,7 @@ ApplyVariables (
>>     UINT32 CpuClock = PcdGet32 (PcdCpuClock);
>>     UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
>>     UINT32 Rate = 0;
>> +  UINT32 ModelFamily = 0;
>>     if (CpuClock != 0) {
>>       if (CpuClock == 2) {
>> @@ -245,51 +249,102 @@ ApplyVariables (
>>       DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
>>     }
>> -  /*
>> -   * Switching two groups around, so disable both first.
>> -   *
>> -   * No, I've not seen a problem, but having a group be
>> -   * routed to two sets of pins seems like asking for trouble.
>> -   */
>> -  GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (48, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (49, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (50, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (51, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (52, GPIO_FSEL_INPUT);
>> -  GpioPinFuncSet (53, GPIO_FSEL_INPUT);
>> -  if (PcdGet32 (PcdSdIsArasan)) {
>> -    DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
>> -    Gpio48Group = GPIO_FSEL_ALT3;
>> +  Status = mFwProtocol->GetModelFamily (&ModelFamily);
>> +  if (Status != EFI_SUCCESS) {
>> +    DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family: 
>> %r\n", Status));
>> +  } else {
>> +    DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is 
>> 0x%x\n", ModelFamily));
>> +  }
>> +
>> +
>> +  if (ModelFamily == 3) {
>>       /*
>> -     * Route SDIO to SdHost.
>> +     * Pi 3: either Arasan or SdHost goes to SD card.
>> +     *
>> +     * Switching two groups around, so disable both first.
>> +     *
>> +     * No, I've not seen a problem, but having a group be
>> +     * routed to two sets of pins seems like asking for trouble.
>>        */
>> -    Gpio34Group = GPIO_FSEL_ALT0;
>> -  } else {
>> -    DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
>> -    Gpio48Group = GPIO_FSEL_ALT0;
>> +    GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (48, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (49, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (50, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (51, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (52, GPIO_FSEL_INPUT);
>> +    GpioPinFuncSet (53, GPIO_FSEL_INPUT);
>> +
>> +    if (PcdGet32 (PcdSdIsArasan)) {
>> +      DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
>> +      Gpio48Group = GPIO_FSEL_ALT3;
>> +      /*
>> +       * Route SDIO to SdHost.
>> +       */
>> +      Gpio34Group = GPIO_FSEL_ALT0;
>> +    } else {
>> +      DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
>> +      Gpio48Group = GPIO_FSEL_ALT0;
>> +      /*
>> +       * Route SDIO to Arasan.
>> +       */
>> +      Gpio34Group = GPIO_FSEL_ALT3;
>> +    }
>> +    GpioPinFuncSet (34, Gpio34Group);
>> +    GpioPinFuncSet (35, Gpio34Group);
>> +    GpioPinFuncSet (36, Gpio34Group);
>> +    GpioPinFuncSet (37, Gpio34Group);
>> +    GpioPinFuncSet (38, Gpio34Group);
>> +    GpioPinFuncSet (39, Gpio34Group);
>> +    GpioPinFuncSet (48, Gpio48Group);
>> +    GpioPinFuncSet (49, Gpio48Group);
>> +    GpioPinFuncSet (50, Gpio48Group);
>> +    GpioPinFuncSet (51, Gpio48Group);
>> +    GpioPinFuncSet (52, Gpio48Group);
>> +    GpioPinFuncSet (53, Gpio48Group);
>> +
>> +  } else if (ModelFamily == 4){
> 
> Missing space before opening brackets.

Thanks for the review. Will fix that.

Regards,

/Pete

> 
> Patch looks good otherwise.
> 
>>       /*
>> -     * Route SDIO to Arasan.
>> +     * Pi 4: either Arasan or eMMC2 goes to SD card.
>>        */
>> -    Gpio34Group = GPIO_FSEL_ALT3;
>> +    if (PcdGet32 (PcdSdIsArasan)) {
>> +      /*
>> +       * WiFi disabled.
>> +       */
>> +      GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> +      GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> +      GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> +      GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> +      GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> +      GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> +      /*
>> +       * SD card pins go to Arasan.
>> +       */
>> +      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> +                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
>> +    } else {
>> +      /*
>> +       * SD card pins back to eMMC2.
>> +       */
>> +      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> +                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
>> +      /*
>> +       * WiFi back to Arasan.
>> +       */
>> +      GpioPinFuncSet (34, GPIO_FSEL_ALT3);
>> +      GpioPinFuncSet (35, GPIO_FSEL_ALT3);
>> +      GpioPinFuncSet (36, GPIO_FSEL_ALT3);
>> +      GpioPinFuncSet (37, GPIO_FSEL_ALT3);
>> +      GpioPinFuncSet (38, GPIO_FSEL_ALT3);
>> +      GpioPinFuncSet (39, GPIO_FSEL_ALT3);
>> +    }
>> +  } else {
>> +    DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n", 
>> ModelFamily));
>>     }
>> -  GpioPinFuncSet (34, Gpio34Group);
>> -  GpioPinFuncSet (35, Gpio34Group);
>> -  GpioPinFuncSet (36, Gpio34Group);
>> -  GpioPinFuncSet (37, Gpio34Group);
>> -  GpioPinFuncSet (38, Gpio34Group);
>> -  GpioPinFuncSet (39, Gpio34Group);
>> -  GpioPinFuncSet (48, Gpio48Group);
>> -  GpioPinFuncSet (49, Gpio48Group);
>> -  GpioPinFuncSet (50, Gpio48Group);
>> -  GpioPinFuncSet (51, Gpio48Group);
>> -  GpioPinFuncSet (52, Gpio48Group);
>> -  GpioPinFuncSet (53, Gpio48Group);
>>     /*
>>      * JTAG pin    JTAG sig    GPIO      Mode    Header pin
>>
>