From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.3527.1588760707495096668 for ; Wed, 06 May 2020 03:25:07 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4B68230E; Wed, 6 May 2020 03:25:06 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.179]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3EFBA3F71F; Wed, 6 May 2020 03:25:05 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic To: Pete Batard , devel@edk2.groups.io Cc: leif@nuviainc.com, andrey.warkentin@gmail.com References: <20200505145029.29826-1-ard.biesheuvel@arm.com> <20200505145029.29826-4-ard.biesheuvel@arm.com> <956d2e2b-440e-9b04-1af1-a4a6a2dbe6ff@akeo.ie> From: "Ard Biesheuvel" Message-ID: <6404e95f-cb15-9924-92bc-8b2b4f7c59c7@arm.com> Date: Wed, 6 May 2020 12:25:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <956d2e2b-440e-9b04-1af1-a4a6a2dbe6ff@akeo.ie> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/6/20 12:18 PM, Pete Batard wrote: > One general remark below: >=20 > On 2020.05.05 19:10, Ard Biesheuvel wrote: >> On 5/5/20 4:50 PM, Ard Biesheuvel wrote: >>> The 16550 'miniUART' on the Raspberry Pi gets its input clock from >>> different sources on RPi3 and RPi3. Fix the logic that derives the >> >> This should be 'Rpi3 and RPi4' >> >>> divisor for the 16550 baud clock on the respective platforms. >>> >>> While at it, make the input clock PCD patchable for RPi3 so we can >>> manipulate it at runtime in a future patch. >=20 > For the sake of harmonization between platforms, wouldn't we want to=20 > also declare the PCD patchable on RPi4 as well (even though we obviously= = =20 > aren't going to patch it for the time being)? >=20 I'd prefer not to. That way, we can easily spot problems when the=20 patching code is incorporated inadvertently. > There is some plan of factorizing the DSC content, so if that doesn't=20 > hurt us, I'd propose we try to keep the Pi3 and Pi4 version as close as= =20 > possible. >=20 > This also ties in with my remark for the next patch. >=20 >>> >>> Co-authored-by: Pete Batard >>> Co-authored-by: Andrei Warkentin >>> Co-authored-by: Ard Biesheuvel >>> Signed-off-by: Pete Batard >>> Signed-off-by: Ard Biesheuvel >>> --- >>> =C2=A0 Platform/RaspberryPi/RPi3/RPi3.dsc |=C2=A0 4 +++- >>> =C2=A0 Platform/RaspberryPi/RPi4/RPi4.dsc |=C2=A0 2 +- >>> =C2=A0 Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLi= b.c=20 >>> | 14 ++++++++------ >>> =C2=A0 3 files changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc=20 >>> b/Platform/RaspberryPi/RPi3/RPi3.dsc >>> index d7218219fc5a..96b27400eef8 100644 >>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc >>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc >>> @@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common] >>> =C2=A0=C2=A0=C2=A0 gArmPlatformTokenSpaceGuid.PL011UartClkInHz|4800000= 0 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRU= E >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStr= ide|4 >>> -=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl= |0x27 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxF= ifoSize|8 >>> =C2=A0=C2=A0=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115= 200 >>> @@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common] >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"= EDK2" >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRU= E >>> +[PcdsPatchableInModule] >>> +=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000 >>> + >>> =C2=A0 [PcdsDynamicHii.common.DEFAULT] >>> =C2=A0=C2=A0=C2=A0 # >>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc=20 >>> b/Platform/RaspberryPi/RPi4/RPi4.dsc >>> index 4fb015b077e6..5d8bd88d7e34 100644 >>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc >>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc >>> @@ -420,7 +420,7 @@ [PcdsFixedAtBuild.common] >>> =C2=A0=C2=A0=C2=A0 gArmPlatformTokenSpaceGuid.PL011UartClkInHz|4800000= 0 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRU= E >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStr= ide|4 >>> -=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000 >>> +=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl= |0x27 >>> =C2=A0=C2=A0=C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxF= ifoSize|8 >>> =C2=A0=C2=A0=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115= 200 >>> diff --git=20 >>> a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c= =20 >>> b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c >>> index b1d17d3fa04a..5e83bbf022eb 100644 >>> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib= .c >>> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib= .c >>> @@ -41,18 +41,20 @@ SerialPortGetDivisor ( >>> =C2=A0=C2=A0=C2=A0 // On the Raspberry Pi, the clock to use for the 16= 650-compatible=20 >>> UART >>> =C2=A0=C2=A0=C2=A0 // is the base clock divided by the 12.12 fixed poi= nt VPU clock=20 >>> divisor. >>> =C2=A0=C2=A0=C2=A0 // >>> -=C2=A0 BaseClockRate =3D (UINT64)PcdGet32 (PcdSerialClockRate) * 4; >>> +=C2=A0 BaseClockRate =3D (UINT64)PcdGet32 (PcdSerialClockRate); >>> +#if (RPI_MODEL =3D=3D 4) >>> =C2=A0=C2=A0=C2=A0 Divisor =3D MmioRead32(BCM2836_CM_BASE +=20 >>> BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF; >>> =C2=A0=C2=A0=C2=A0 if (Divisor !=3D 0) >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 BaseClockRate =3D (BaseClockRate << 12)= / Divisor; >>> +#endif >>> =C2=A0=C2=A0=C2=A0 // >>> -=C2=A0 // Now calculate divisor for baud generator >>> -=C2=A0 //=C2=A0=C2=A0=C2=A0 Ref_Clk_Rate / Baud_Rate / 16 >>> +=C2=A0 // As per the BCM2xxx datasheets: >>> +=C2=A0 // baudrate =3D system_clock_freq / (8 * (divisor + 1)). >>> =C2=A0=C2=A0=C2=A0 // >>> -=C2=A0 Divisor =3D (UINT32)BaseClockRate / (SerialBaudRate * 16); >>> -=C2=A0 if (((UINT32)BaseClockRate % (SerialBaudRate * 16)) >=3D=20 >>> SerialBaudRate * 8) { >>> -=C2=A0=C2=A0=C2=A0 Divisor++; >>> +=C2=A0 Divisor =3D (UINT32)BaseClockRate / (SerialBaudRate * 8); >>> +=C2=A0 if (Divisor !=3D 0) { >>> +=C2=A0=C2=A0=C2=A0 Divisor--; >>> =C2=A0=C2=A0=C2=A0 } >>> =C2=A0=C2=A0=C2=A0 return Divisor; >>> =C2=A0 } >>> >> >> >>=20 >> >=20 > Reviewed-by: Pete Batard >=20