Re: [edk2-platforms][PATCH v2 1/7] Silicon/Bcm283x: Add UART constants for PL011 and miniUART

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/19/19 2:32 PM, Ard Biesheuvel wrote:
> On Thu, 19 Dec 2019 at 15:27, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 12/19/19 1:14 PM, Pete Batard wrote:
>>> This adds offset, base address, interrupt and register-space length
>>> for the 2 UARTs that the Bcm283x SoC provides.
>>>
>>> To be consistent, we simplify the two other existing base address
>>> definitions to a more legible equivalent since there is no point
>>> in explicit refs to FixedPcdGet64 (PcdBcm283xRegistersAddress).
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> ---
>>>    Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h | 17 +++++++++++++----
>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> index 02107086d439..dd9a698f7218 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> +++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> @@ -24,8 +24,7 @@
>>>
>>>    /* watchdog constants */
>>>    #define BCM2836_WDOG_OFFSET                                 0x00100000
>>> -#define BCM2836_WDOG_BASE_ADDRESS                           (FixedPcdGet64 (PcdBcm283xRegistersAddress) \
>>> -                                                            + BCM2836_WDOG_OFFSET)
>>> +#define BCM2836_WDOG_BASE_ADDRESS                           (BCM2836_SOC_REGISTERS + BCM2836_WDOG_OFFSET)
>>
>> Good!
>>
>>>    #define BCM2836_WDOG_PASSWORD                               0x5a000000
>>>    #define BCM2836_WDOG_RSTC_OFFSET                            0x0000001c
>>>    #define BCM2836_WDOG_WDOG_OFFSET                            0x00000024
>>> @@ -34,8 +33,7 @@
>>>
>>>    /* mailbox interface constants */
>>>    #define BCM2836_MBOX_OFFSET                                 0x0000b880
>>> -#define BCM2836_MBOX_BASE_ADDRESS                           (FixedPcdGet64 (PcdBcm283xRegistersAddress) \
>>> -                                                            + BCM2836_MBOX_OFFSET)
>>> +#define BCM2836_MBOX_BASE_ADDRESS                           (BCM2836_SOC_REGISTERS + BCM2836_MBOX_OFFSET)
>>>    #define BCM2836_MBOX_READ_OFFSET                            0x00000000
>>>    #define BCM2836_MBOX_STATUS_OFFSET                          0x00000018
>>>    #define BCM2836_MBOX_CONFIG_OFFSET                          0x0000001c
>>> @@ -50,4 +48,15 @@
>>>    #define BCM2836_INTC_TIMER_CONTROL_OFFSET                   0x00000040
>>>    #define BCM2836_INTC_TIMER_PENDING_OFFSET                   0x00000060
>>>
>>> +/* uart constants */
>>> +#define BCM2836_PL011_UART_OFFSET                           0x00201000
>>> +#define BCM2836_PL011_UART_BASE_ADDRESS                     (BCM2836_SOC_REGISTERS + BCM2836_PL011_UART_OFFSET)
>>> +#define BCM2836_PL011_UART_LENGTH                           0x00001000
>>> +#define BCM2836_PL011_UART_INTERRUPT                        0x99
>>
>> Hmm 96 + 29 = 0x7D, 96 + 57 = 0x99. OK.
>> Maybe we can later add a definition for this '96'.
>>
> 
> Ehm, no that's not going to happen. It is fine to abstract actual
> physical properties like the base of a register block, but that
> doesn't make it an end in itself to express arbitrary values like
> interrupt line numbers like this.

OK, understood.

>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
> 
> Thanks!
> 
> 
>>> +
>>> +#define BCM2836_MINI_UART_OFFSET                            0x00215000
>>> +#define BCM2836_MINI_UART_BASE_ADDRESS                      (BCM2836_SOC_REGISTERS + BCM2836_MINI_UART_OFFSET)
>>> +#define BCM2836_MINI_UART_LENGTH                            0x00000070
>>> +#define BCM2836_MINI_UART_INTERRUPT                         0x7D
>>> +
>>>    #endif /*__BCM2836_H__ */
>>>
>>
>