From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.8340.1574872413201753124 for ; Wed, 27 Nov 2019 08:33:33 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=qZldcgSu; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.66, mailfrom: pete@akeo.ie) Received: by mail-wm1-f66.google.com with SMTP id g206so7778335wme.1 for ; Wed, 27 Nov 2019 08:33:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=cWPVJCJrM/osVcAKvsZ+9MAz9mer6vDXM19ajKHTrqY=; b=qZldcgSuJDts9PuvsG9C3l302Ac7TEFcAME6I2cxFyEJi5bho40vVckxSfEH12kMEs X7lsjg74r0i1WCdurbRPc+EOJjkc83wfroE64+EMIrfcKOSWa1un5apkEUoVlolKNe8P 24rtGPXebIe8w6KvNRFW9LGzmbVV0g0vyoqdq3rxvAxALijX5xcOaYTCtLexbhnUXKgx K94zTjVXo+eMjdPeAOm/r0sd5wLTagvm1SjK86yYnepWhrJa7806lH11+5NJxSP9no4Q BYomjPd8fMcO3LUxwH2zLcMNoYYY3Ml4w1RV44VS4WoQHsJThoRQolc9Yfvn+NVgE/NS 0PQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=cWPVJCJrM/osVcAKvsZ+9MAz9mer6vDXM19ajKHTrqY=; b=iJ029FcLBKwYF/3IoH+OGYPqyuDyD/IlaLywFbHsGwKAf8blKChI0CGekamr1B2SFF F6QL2kLiKxsWqWE3eRbaEU3k4ekoS3SODSFy4lX/zEG5ZarXvg+U6QwO00Atjvg0G2b3 ufZuEVW7bkhjZh4AH99COCbJFWXE3PuqogkHU4ZsZ0NdZ4H6Lpom57XSs0oPArKH5tS/ tnnk+U6OtZyZ9RalOtnBoe8OVXXw6AtQgkabz3EuP0sYz0F0+eYF2VT8n/6vJtZGz+F9 UapmP34Gd73KPHc7u39wmV1HZ4KOIHDZZj365yJYOXi4PYvdJlwogIv2W5NohmsYEiVu LBtA== X-Gm-Message-State: APjAAAW1Advd4ugTouSku/3B5poEAiT1e4enm5qsWLRXHBuyPgk1hwRG 03Kq7MjgtFRpzkvZBj6rTgpn1A== X-Google-Smtp-Source: APXvYqzd9iNkpbaLxexDdYT5VPcvXAGeZVBwRPOb6YlEEmRAy5Ih+8JjXJ559R1NaK9WO0ppStpi2g== X-Received: by 2002:a7b:ce92:: with SMTP id q18mr5432145wmj.164.1574872411583; Wed, 27 Nov 2019 08:33:31 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.37.1]) by smtp.googlemail.com with ESMTPSA id z2sm5181894wmf.47.2019.11.27.08.33.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Nov 2019 08:33:30 -0800 (PST) Subject: Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io Cc: ard.biesheuvel@linaro.org, leif.lindholm@linaro.org, samer.el-haj-mahmoud@arm.com, andrey.warkentin@gmail.com References: <20191127123706.4604-1-pete@akeo.ie> <20191127123706.4604-6-pete@akeo.ie> <672a8fd3-f9db-3d60-ea02-188af6e8a6a5@redhat.com> From: "Pete Batard" Message-ID: <514e89d4-d571-b89b-0598-e6a56de93215@akeo.ie> Date: Wed, 27 Nov 2019 16:33:28 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: <672a8fd3-f9db-3d60-ea02-188af6e8a6a5@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit 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 >> >> 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 >> --- >>   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 >> + *  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 >> >>    * >>    *  SPDX-License-Identifier: BSD-2-Clause-Patent >>    * >> @@ -9,10 +10,12 @@ >>   #include >>   #include >>   #include >> +#include >>   #include >>   #include >>   #include >>   #include >> +#include >>   #include >>   #include >>   #include >> @@ -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 >> >