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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

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.

> 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__ */
> >
>